Closed
Bug 773151
Opened 12 years ago
Closed 12 years ago
Rename nsCAutoString to nsAutoCString
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Keywords: dev-doc-needed)
Attachments
(3 files, 5 obsolete files)
Convert nsCAutoString to nsAutoCString, to make it in line with how we name other classes/etc.
[23:20] njn wishes nsCAutoString was called nsAutoCString
[23:25] gavin too
[23:26] roc fix it
[23:28] njn roc: MXR says "Too many hits, displaying the first 1000"
[23:28] njn sighs
[23:31] roc search-and-replace
[23:34] darktrojan sed
[23:35] cpearce FTW!
[23:46] jesup roc: Wonder how well the tree will compile after this: 'chfiles | xargs sed -e "s/nsCAutoString/nsAutoCString/" --in-place' (and the same with ifiles). (chfiles - finds all .c/cpp/c/h files; ifiles finds idl files)
[23:47] roc something like that
[23:47] jcranmer jesup: you'd have to apply it to comm-central
[23:47] khuey why are we renaming this?
[23:47] jcranmer or, more likely, put a typedef in an exported header so other projects won't immediately break
[23:50] roc khuey: because nsAutoFooBar is common, and nsFooAutoBar is not
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Comment on attachment 641359 [details] [diff] [review]
convert nsCAutoString to nsAutoCString
bitrot on this is likely, but it's easy to recreate
Attachment #641359 -
Flags: review?(benjamin)
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 641360 [details] [diff] [review]
provide back-compatibility for (source) code using nsCAutoString
Untested - to keep comm-central and other codebases happy (if something more is needed, let me know. Seamonkey would be another likely impactee.)
Attachment #641360 -
Flags: review?(benjamin)
Assignee | ||
Comment 5•12 years ago
|
||
Comment 7•12 years ago
|
||
Better late than never!
Assignee | ||
Comment 8•12 years ago
|
||
Documentation changes will be needed if we take this!
Comment 9•12 years ago
|
||
FWIW, I am totally in favor of this change. And "nsAutoCString" is the same length as "nsCAutoString", so we don't have to worry about breaking indentation.
But I'm worried about leaving the |#define nsCAutoString nsAutoCString| in there, because people will continue to (accidentally) use nsCAutoString. Can we restrict the #define to non-Mozilla-core code?
Assignee | ||
Comment 10•12 years ago
|
||
I'd love to do so; if someone can tell me how. Or throw (compile-time) warnings when someone uses it.
Comment 11•12 years ago
|
||
You could put the compatibility define at the end of xpcom/glue/nsStringAPI.h, which is a header beginning with:
#ifdef MOZILLA_INTERNAL_API
#error nsStringAPI.h is only usable from non-MOZILLA_INTERNAL_API code!
#endif
(Most of the internal string headers begin with the opposite.)
Comment 12•12 years ago
|
||
Comment on attachment 641359 [details] [diff] [review]
convert nsCAutoString to nsAutoCString
This sounds good, but I think you should announce this in the newsgroups first and inform people how to update their in-progress patches with sed.
Attachment #641359 -
Flags: review?(benjamin) → review+
Comment 13•12 years ago
|
||
Comment on attachment 641360 [details] [diff] [review]
provide back-compatibility for (source) code using nsCAutoString
This provides back-compatibility for internal-api consumers, which I'm not sure we want. We should definitely have either a typedef or a #define for the external-api (nsStringAPI.h). Are you planning on making this temporary while comm-central and any in-progress patches/feature branches get updated?
Attachment #641360 -
Flags: review?(benjamin) → review+
I don't think internal-API consumers using nsCAutoString is too likely to be a problem, since most people are just going to copy their definition from somewhere else. But comment #11 makes sense.
Assignee | ||
Comment 15•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #641360 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 641743 [details] [diff] [review]
provide back-compatibility for (source) code using nsCAutoString (updated)
Updated per comment 11; using typedef as well
Attachment #641743 -
Flags: review?(benjamin)
Updated•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #641743 -
Attachment is obsolete: true
Attachment #641743 -
Flags: review?(benjamin)
Assignee | ||
Updated•12 years ago
|
Attachment #642174 -
Flags: review?(benjamin)
Updated•12 years ago
|
Attachment #642174 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 18•12 years ago
|
||
Ok, since we are doing an uplift, I'm preparing an updated masschange patch to land this...
Assignee | ||
Comment 19•12 years ago
|
||
Assignee | ||
Comment 20•12 years ago
|
||
Comment on attachment 655580 [details] [diff] [review]
convert nsCAutoString to nsAutoCString
may have been corrupted by an interrupted upload
Attachment #655580 -
Attachment is obsolete: true
Assignee | ||
Comment 21•12 years ago
|
||
patch is the result of: chfiles | xargs sed -e 's/nsCAutoString/nsAutoCString/' --in-place
Assignee | ||
Updated•12 years ago
|
Attachment #641359 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #655581 -
Flags: review?(benjamin)
Assignee | ||
Comment 22•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c175647627b8
To convert all the patch queues in a repo, do this:
WARNING: back up or push your patches before doing this! YOU HAVE BEEN WARNED!
(ok, overly melodramatic, but please don't risk losing data; you probably can just do "cp -a .hg/patches* /tmp/backup_patches" for a simple backup)
find .hg -path ".hg/patches*" -type f -print | xargs sed -e 's/nsCAutoString/nsAutoCString/' --in-place
Comment 23•12 years ago
|
||
Comment on attachment 655581 [details] [diff] [review]
convert nsCAutoString to nsAutoCString
The script does its thing, I can't really review this anyway.
Attachment #655581 -
Flags: review?(benjamin)
Assignee | ||
Comment 24•12 years ago
|
||
Here's the more detailed and pedantic way to update patches and repos I posted on m.dev.platform:
If you want to convert directories of patches:
# NOTE: for *BSD or Mac, use -i instead of --in-place
# BACK UP YOUR PATCHES FIRST! (or have them under repository control)
hg qpop -a
# Pick up the nsAutoCString changes
hg pull -u
cd .hg/patches
# repeat for patches-whatever for each qqueue you have
# You could wrap this in something that finds all your patches*
# directories and runs this.
find . -not -name 'series' -and -not -name 'status' -and -not -name ".*" -print | xargs sed -e "s/nsCAutoString/nsAutoCString/g" --in-place
For codebases: to generate a patch to convert them:
# you may need to tweak this find command.... in particular, excluding
# object directories!!! We assume they're named obj*
find -name '.hg' -prune , -name 'obj*' -prune , -type f -and -not -name '*~' -and -not -name ".*" -print | xargs sed -e "s/nsCAutoString/nsAutoCString/g" --in-place
Or, if you're paranoid about hitting binaries/html/etc:
# you may need to tweak this find command.... in particular, excluding
# object directories!!! We assume they're named obj*
find -name '.hg' -prune , -name 'obj*' -prune , \( -name "*.c" -o -name "*.cpp" -o -name "*.h" -o -name "*.cc" -o -name "*.mm" \) -type f -print | xargs sed -e "s/nsCAutoString/nsAutoCString/g" --in-place
# I used something like this to convert inbound to create the patch
If there are errors above, my apologies - I did try these out (but only
on a Fedora system).
Comment 25•12 years ago
|
||
It might be easiest for everybody if you also took one of Ehsan's bash scripts and modified it for usage, and uploaded it somewhere. It has various nice features like checking that you have popped all of your patches, and we've used variations of that script multiple times before so we know what to expect. Thanks.
Assignee | ||
Comment 26•12 years ago
|
||
Comment on attachment 655581 [details] [diff] [review]
convert nsCAutoString to nsAutoCString
Run with the wrong script; the right one is the one I just posted for converting a repo (which was in .platform, but not here before). Also, the patch itself isn't interesting to have on the bug, as the script really needs to be run on a closed tree and then checked in.
Attachment #655581 -
Attachment is obsolete: true
Assignee | ||
Comment 27•12 years ago
|
||
I ran this script and diffed the resultant patch against the one I had before - no differences.
Updated•12 years ago
|
Attachment #655621 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 28•12 years ago
|
||
Modified from Ehsan's bitrot patch - main change is to have it function on *all* patch queues, not just the default .hg/patches queue
Tested on a local repo with 10 queues
Attachment #655673 -
Flags: review?(benjamin)
Updated•12 years ago
|
Attachment #655673 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 29•12 years ago
|
||
Per discussion with joduinn, targeted to be done during the Oct 8th migration/closure (see blocks above).
Comment 30•12 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #29)
> Per discussion with joduinn, targeted to be done during the Oct 8th
> migration/closure (see blocks above).
After further discussions, jesup proposed in today's platform mtg that he would do this in the coming week or so, during "a lull in checkins", instead of waiting for next migration. Most likely a Friday evening or sometime over a weekend. More info as we have it.
Leaving link to bug#785991, until new date is confirmed.
Comment 31•12 years ago
|
||
Randell, if we want to avoid landing the typedef due to comm-central, I'm quite happy to see if we can get someone to sync up with you on the landing and just do both repos at the same time.
Assignee | ||
Comment 32•12 years ago
|
||
Plan is to land late this evening or Saturday (or earlier Sunday so as not to affect New Zealand employees). Removing link.
Agreed, we should do both repos together. Mark, what's required and when would be a good time for you this weekend?
No longer blocks: 785991
Assignee | ||
Comment 33•12 years ago
|
||
Comment 34•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•4 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•