Closed
Bug 1050107
Opened 10 years ago
Closed 10 years ago
Removal of file "nss/lib/certhigh/certvfypkixprint.c" from NSS compilation, to remove deadcode.
Categories
(NSS :: Libraries, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
3.17.1
People
(Reporter: sachingpt999, Assigned: sachingpt999)
References
Details
Attachments
(1 file, 1 obsolete file)
12.56 KB,
patch
|
wtc
:
review+
wtc
:
checked-in+
|
Details | Diff | 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.
Assignee | ||
Updated•10 years ago
|
Attachment #8469073 -
Flags: review?(ryan.sleevi)
Comment 1•10 years ago
|
||
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+
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Assignee: nobody → s.k.gupta
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
Reminder for code check in.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(wtc)
Updated•10 years ago
|
Flags: needinfo?(wtc)
Flags: needinfo?(ryan.sleevi)
Flags: needinfo?(kaie)
Comment 5•10 years ago
|
||
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.
Comment 6•10 years ago
|
||
Looking for a third opinion (delete or keep) from either Wan-Teh or Bob.
Keywords: checkin-needed
Updated•10 years ago
|
Attachment #8469073 -
Flags: superreview?(wtc)
Attachment #8469073 -
Flags: review?(rrelyea)
Attachment #8469073 -
Flags: feedback-
Assignee | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
(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.
Comment 9•10 years ago
|
||
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
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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.
Comment 12•10 years ago
|
||
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+
Updated•10 years ago
|
Severity: normal → trivial
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Priority: -- → P3
Resolution: --- → FIXED
Whiteboard: [3.17.1]
Target Milestone: --- → 3.17
Updated•10 years ago
|
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.
Description
•