Closed Bug 1050107 Opened 7 years ago Closed 7 years ago

Removal of file "nss/lib/certhigh/certvfypkixprint.c" from NSS compilation, to remove deadcode.

Categories

(NSS :: Libraries, defect, P3)

3.16.3
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.17.1

People

(Reporter: sachingpt999, Assigned: sachingpt999)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Proposed Patch (obsolete) — Splinter Review
Spinoff from https://bugzilla.mozilla.org/show_bug.cgi?id=1047288#c1 , 

ryan.sleevi@gmail.com suggests the removal of the file certvfypkixprint.c, as 
it defines functions used within the #ifdef DEBUG_volkov macro only (in certvfypkix.c). 

Attached patch 

- removes the file nss/lib/certhigh/certvfypkixprint.c, 

- removes the code within #ifdef DEBUG_volkov in   nss/lib/certhigh/certvfypkix.c,

- and modifies nss/lib/certhigh/manifest.mn, to remove compilation of certvfypkixprint.c entirely.

Patch compiles cleanly with 3.16.3 release, 
and no regressions in testsuite are noticed.

Relevant bug 1047288.
Attachment #8469073 - Flags: review?(ryan.sleevi)
Comment on attachment 8469073 [details] [diff] [review]
Proposed Patch

Review of attachment 8469073 [details] [diff] [review]:
-----------------------------------------------------------------

Great stuff! Nuking dead code is always a win :)
Attachment #8469073 - Flags: review?(ryan.sleevi) → review+
Keywords: checkin-needed
Assignee: nobody → s.k.gupta
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Thanks Ryan. But I don't have check in permissions as I am not a mozilla committer, it would be good it you could point someone to vouch me for commit permissions or assign some one else for code check in.
Flags: needinfo?(ryan.sleevi)
Reminder for code check in .
Flags: needinfo?(kaie)
Reminder for code check in.
Flags: needinfo?(wtc)
Flags: needinfo?(wtc)
Flags: needinfo?(ryan.sleevi)
Flags: needinfo?(kaie)
I'm not convinced this change is a good idea.

The code being removed is debugging code. Because libpkix is very complicated, Alexei Volkov obviously had produced code to trace the behaviour at runtime. The code is explicitly disabled at build time.

In my opinion, this isn't "forgotten" code, rather, it's code that could be enabled by a developer, while learning the code, or while investigating an issue.

Since this code doesn't cause any harm, but is completely wrapped in #ifdef DEBUG_ blocks, I'd recommend to keep it.
Looking for a third opinion (delete or keep) from either Wan-Teh or Bob.
Keywords: checkin-needed
Attachment #8469073 - Flags: superreview?(wtc)
Attachment #8469073 - Flags: review?(rrelyea)
Attachment #8469073 - Flags: feedback-
Kai, the file certvfypkixprint.c gets compiled even during non-debug mode, i think it would be better if we keep the file , but limit its compilation to debug mode by making changes in manifest.mn placed inside certhigh.
Flags: needinfo?(kaie)
(In reply to Kai Engert (:kaie) from comment #5)
> I'm not convinced this change is a good idea.
> 
> The code being removed is debugging code. Because libpkix is very
> complicated, Alexei Volkov obviously had produced code to trace the
> behaviour at runtime. The code is explicitly disabled at build time.
> 
> In my opinion, this isn't "forgotten" code, rather, it's code that could be
> enabled by a developer, while learning the code, or while investigating an
> issue.
> 
> Since this code doesn't cause any harm, but is completely wrapped in #ifdef
> DEBUG_ blocks, I'd recommend to keep it.

This is exactly what prevents people from wanting to commit to or use NSS.

Orphaned code is bad. Version control has history for a reason. We should not react against NSS-Rampage, but encourage it.
To save those quickly reviewing: This was landed in 2007. As a developer-specific debug flag. For a bug that is obsolete.

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/certvfypkix.c&rev=1.56&mark=30#30
I think this issue is too minor to turn this into an emotional debate, I'm surprised that my suggestion triggered comments like the one in bug 1047288 comment 10.

I think it doesn't matter that the original author of this debug code isn't working on NSS any longer. Obviously we don't want to remove people's code simply because they have stopped contributing.

The code here was valuable for me in the past. I remember that I had enabled it in the past, when I tried to understand the flow inside libpkix. It was easy to find and easy to enable. I certainly wouldn't have found it, it it had been buried in past revisions of the version control system, only.

I don't get your argument that code disabled at build time, which is also clearly marked as debug-only code, would be able to cause any harm. If you dislike that some code is wrapped using DEBUG, we could wrap all of it using a symbol that isn't used in regular debug builds, like DEBUG_old.

Yes, in general I completely agree that using old, dead code should be removed, if it's not being used any longer. But IMHO optional debug code is a different story, as it has educational and documentary purposes.

That's my opinion. I didn't try to force my opinion on anyone. I have encouraged getting another opinion on this by asking Bob and Wan-Teh for their thoughts. That's all.

Receiving blame for bringing up new arguments is discouraging.
Flags: needinfo?(kaie)
Comment on attachment 8469073 [details] [diff] [review]
Proposed Patch

Review of attachment 8469073 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/certhigh/certvfypkixprint.c
@@ -9,5 @@
> - */
> -#include "cert.h"
> -#include "pkix_pl_common.h"
> -
> -#ifdef DEBUG

We should at least change this DEBUG to DEBUG_volkov.

I usually don't use other people's debugging code, so I am
fine with removing this code.
I deleted some blank lines that Sachin neglected to delete.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/40f8194e0d7a
Attachment #8469073 - Attachment is obsolete: true
Attachment #8469073 - Flags: superreview?(wtc)
Attachment #8469073 - Flags: review?(rrelyea)
Attachment #8472690 - Flags: review+
Attachment #8472690 - Flags: checked-in+
Severity: normal → trivial
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Priority: -- → P3
Resolution: --- → FIXED
Whiteboard: [3.17.1]
Target Milestone: --- → 3.17
Duplicate of this bug: 1047288
Whiteboard: [3.17.1]
Target Milestone: 3.17 → 3.17.1
You need to log in before you can comment on or make changes to this bug.