Closed Bug 773151 Opened 8 years ago Closed 8 years ago
CAuto String to ns Auto CString
832 bytes, patch
|Details | Diff | Splinter Review|
579 bytes, text/plain
875 bytes, text/plain
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
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)
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)
Yeah, we should have done this as a followup to bug 69873. :-)
Better late than never!
Documentation changes will be needed if we take this!
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?
I'd love to do so; if someone can tell me how. Or throw (compile-time) warnings when someone uses it.
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 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 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.
Attachment #641360 - Attachment is obsolete: true
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 #642174 - Flags: review?(benjamin)
Ok, since we are doing an uplift, I'm preparing an updated masschange patch to land this...
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
patch is the result of: chfiles | xargs sed -e 's/nsCAutoString/nsAutoCString/' --in-place
Attachment #641359 - Attachment is obsolete: true
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 on attachment 655581 [details] [diff] [review] convert nsCAutoString to nsAutoCString The script does its thing, I can't really review this anyway.
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).
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.
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
I ran this script and diffed the resultant patch against the one I had before - no differences.
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Attachment #655621 - Flags: review?(benjamin)
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)
8 years ago
Per discussion with joduinn, targeted to be done during the Oct 8th migration/closure (see blocks above).
(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.
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.
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.