Last Comment Bug 398019 - correct confusing and erroneous comments in DER_AsciiToTime
: correct confusing and erroneous comments in DER_AsciiToTime
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.0
: All All
: P2 minor (vote)
: 3.12
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
Depends on:
Blocks: 398693
  Show dependency treegraph
 
Reported: 2007-09-29 14:19 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2008-06-20 13:00 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (3.06 KB, patch)
2007-09-29 14:19 PDT, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
julien.pierre: review+
Details | Diff | Review

Description Nelson Bolyard (seldom reads bugmail) 2007-09-29 14:19:27 PDT
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.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2007-09-29 14:23:59 PDT
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.  
Comment 2 Robert Relyea 2007-10-02 10:58:16 PDT
Comment on attachment 282848 [details] [diff] [review]
patch v1

r+ with your comment update.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2007-10-05 01:33:42 PDT
Checking in lib/util/dertime.c; new revision: 1.9; previous revision: 1.8
Comment 4 Nelson Bolyard (seldom reads bugmail) 2007-10-05 12:54:11 PDT
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.
Comment 5 Julien Pierre 2007-10-09 14:01:59 PDT
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.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2007-10-22 17:10:54 PDT
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.  

Note You need to log in before you can comment on or make changes to this bug.