Closed Bug 326317 Opened 16 years ago Closed 9 years ago

issues with comments in nsTSubstring.h

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dbaron, Assigned: Cykesiopka)

References

Details

(Keywords: helpwanted, Whiteboard: [mentor=bsmedberg][lang=C++])

Attachments

(1 file, 1 obsolete file)

Two issues with the comments in nsTSubstring.h:

(1) SetCapacity should have a comment explaining that the capacity argument does not need to include room for a null terminator; it's the job of the string class to account for that.  (I always have to read code to double-check that.  It might also be good to explain that the number is in characters, not bytes.)

(2) The comments:
    // EqualsLiteral must ONLY be applied to an actual literal string.
    // Do not attempt to use it with a regular char* pointer, or with a char
    // array variable.
and
    // LowerCaseEqualsLiteral must ONLY be applied to an actual
    // literal string.  Do not attempt to use it with a regular char*
    // pointer, or with a char array variable. Use
    // LowerCaseEqualsASCII for them.
and
    // AssignLiteral must ONLY be applied to an actual literal string.
    // Do not attempt to use it with a regular char* pointer, or with a char
    // array variable. Use AssignASCII for those.
should be weakened to make it clear that character array *constants* declared without explicit sizes are allowed.  There are some uses in the tree, such as many of:
http://lxr.mozilla.org/seamonkey/search?string=EqualsLiteral%28k
(although some are probably macros), and I don't see any reason it should be forbidden.
Keywords: helpwanted
Priority: -- → P3
Whiteboard: [mentor=bsmedberg][lang=C++]
Attached patch Proposed Patch v1 (obsolete) — Splinter Review
Copied some of your wording :dbaron, hope you don't mind...

I noticed that AppendLiteral() has a similar comment, but I wasn't sure if the same weakening should be applied there...
Assignee: nobody → cykesiopka
Status: NEW → ASSIGNED
Attachment #716937 - Flags: review?(dbaron)
Comment on attachment 716937 [details] [diff] [review]
Proposed Patch v1

r=dbaron, thanks for fixing

Perhaps also mention in the comment for EqualsLiteral, at the end, "Use EqualsASCII for them.", just as at the end of the other comments?

(Feel free to add that change without re-requesting review, post a new patch, and then mark as checkin-needed.)
Attachment #716937 - Flags: review?(dbaron) → review+
Patch for check in, includes "Use EqualsASCII for them." line.
Attachment #716937 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7b1ac19bfac
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 1499212
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.