326317 - issues with comments in nsTSubstring.h

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[:Cykesiopka]

Attachment: Proposed Patch v1

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...

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

( See https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in )

Comment 4

[:Cykesiopka]

Attachment: Patch for Check In

Patch for check in, includes "Use EqualsASCII for them." line.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b1ac19bfac

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7b1ac19bfac