correct confusing and erroneous comments in DER_AsciiToTime

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
minor
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 282848 [details] [diff] [review]
patch v1

While investigating bug 397968, I found that the source code for 
DER_AsciiToTime contains numerous comments that are confusing at best
or downright incorrect.  I also found uses of time_t that are 
unnecessary and harmless, but create confusion and suspicion.

The code appears to be entirely correct, but with the present comments
and odd type casts, it is difficult for a reviewer to SEE that it is 
correct without doing a lot of work.

So, I wrote a patch to correct the comments, and to eliminate the use
of time_t.  This patch does not change the actual computations or 
behavior any.  Its only purpose is to make the code more obviously 
correct to a reviewer.
Attachment #282848 - Flags: review?(rrelyea)
(Assignee)

Comment 1

10 years ago
I need to correct one new comment line as follows:
- /* compute seconds in elapsed leap days since 1970 */
+ /* compute elapsed leap days since 1970 */

I need to think some more about whether that computation is correct for
years between 1950 and 1969.  I'm not sure that it is.  
Status: NEW → ASSIGNED

Comment 2

10 years ago
Comment on attachment 282848 [details] [diff] [review]
patch v1

r+ with your comment update.
Attachment #282848 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 3

10 years ago
Checking in lib/util/dertime.c; new revision: 1.9; previous revision: 1.8
(Assignee)

Updated

10 years ago
Blocks: 398693
(Assignee)

Comment 4

10 years ago
Comment on attachment 282848 [details] [diff] [review]
patch v1

requesting second review for branch.  This patch is a precursor to the patch for bug 398693.
Attachment #282848 - Flags: review?(julien.pierre.boogz)

Comment 5

10 years ago
Comment on attachment 282848 [details] [diff] [review]
patch v1

This patch is correct, but for the new code, I would rather replace the old LL_I2L macro with something more readable, since it isn't needed anymore.
Attachment #282848 - Flags: review?(julien.pierre.boogz) → review+
(Assignee)

Comment 6

10 years ago
I'm marking this bug fixed.  The patch ws committed on the trunk, but
not on the branch.  Then I discovered bug 398693 which completely 
rewrites these functions.  So, there remains no need for this bug's patch 
to be committed on the branch.  
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: 3.11.8 → 3.12
(Assignee)

Updated

9 years ago
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.