nsAuto[C]String contain 64 chars inline. This is usually fine, but occasionally we'd like to specify a different number of chars. We can do this with templates.

This patch parameterizes nsAuto[C]String, renames them as nsAuto[C]StringN, and redefines nsAuto[C]String as typedefs for nsAuto[C]StringN<64>. (The alternative would be to templatize nsAuto[C]String and use a default parameter, but that would require writing "nsAuto[C]String<>" everywhere.)

In all of these cases the fixed buffer has the same lifetime as the string object, so we can use nsAuto[C]String for simplicity. For the 128-length ones in dom/xul/ I just switched to the default of 64 for simplicity, because the choice of 128 didn't seem that important. Finally, the patch also changes LoggingIdString to use nsAutoCStringN<NSID_LENGTH>, similar to NullPrincipalURI.

I'm wondering if there are better naming options here. I'm not crazy about the "N" name. (Would reusing the FixedString name be a good idea or not?)

I deliberately left the default 64-char cases as nsAuto[C]String, to avoid massive amounts of churn. It makes sense for the class name to be something similar to nsAuto[C]String. erahm was using nsBaseAuto[C]String at one point. I chose nsAuto[C]StringN because it mirrors nsAuto[C]String<N>. TBH I don't think it matters all that much because the default 64-char case is so much more common.

I would like to encourage people to use different sizes a bit more than today, though.

(In reply to David Baron :dbaron: ⌚️UTC+2 from comment #5) > I would like to encourage people to use different sizes a bit more than > today, though. Do we think we have a good handle on how long strings typically are at given callsites, though? For nsAutoTArray, we can usually say "1" or "4" or whichever; I don't know that we have that same intuition around strings. (Though std::string implementations seem to have converged on something much less than 64, so maybe we should bump ours down anyway!)

> Do we think we have a good handle on how long strings typically are at given > callsites, though? I don't know. They're mostly on the stack, in which case it maybe doesn't matter that much, so long as functions aren't recursive? But changing the default size is beyond the scope of this bug. Anyway, the main sticking point seems to be the class name. The names I considered: - nsBaseAuto[C]String - nsAutoN[C]String - nsAuto[C]StringN I thought the last one was the best, so I used it in the patch. I'm happy to hear other suggestions.

(In reply to David Baron :dbaron: ⌚️UTC+2 from comment #3) > I'm wondering if there are better naming options here. I'm not crazy about > the "N" name. (Would reusing the FixedString name be a good idea or not?) FixedString sounds like something that can't change size, nsAutoString definitely can. (In reply to Nicholas Nethercote [:njn] from comment #7) > Anyway, the main sticking point seems to be the class name. The names I > considered: > > - nsBaseAuto[C]String > > - nsAutoN[C]String > > - nsAuto[C]StringN > > I thought the last one was the best, so I used it in the patch. I'm happy to > hear other suggestions. nsAutoN[C]String aligns well with nsAutoTArray. I'm not sure if that's a compelling argument though.

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #8) > nsAutoN[C]String aligns well with nsAutoTArray. I'm not sure if that's a > compelling argument though. It has the disadvantage of having a bunch of capital letters in a row that could be hard to parse, especially with nsAutoNCString. I guess I'm inclined to just go with the names in njn's current patch.

Comment on attachment 8892382 [details] [diff] [review] (part 2) - Convert nsFixed[C]String uses to nsAuto[C]String Review of attachment 8892382 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting review request.

Comment on attachment 8892382 [details] [diff] [review] (part 2) - Convert nsFixed[C]String uses to nsAuto[C]String Review of attachment 8892382 [details] [diff] [review]: ----------------------------------------------------------------- r=me with confirmation that switching to 64 is fine. ::: caps/NullPrincipalURI.cpp @@ +39,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + mPath.SetLength(NSID_LENGTH - 1); // -1 because NSID_LENGTH counts the '\0' > + id.ToProvidedString( > + *reinterpret_cast<char(*)[NSID_LENGTH]>(mPath.BeginWriting())); This is rough, I wonder if we could add a `GetFixedBuffer` to nsAutoCStringN in a follow up or `nsID::ToProvidedString(nsAutoCStringN<NSID_LENGTH>&)`. ::: dom/xul/templates/nsXULContentBuilder.cpp @@ +599,5 @@ > // actual value of the 'rdf:resource' attribute for the > // given node. > // SynchronizeUsingTemplate contains code used to update textnodes, > // so make sure to modify both when changing this > + nsAutoString attrValue; This is shrinking from 128 -> 64, I know you thought it was arbitrary but is it possible to dump the string sizes on startup and confirm?

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f9ec011c9bd5da36b51cb09a81151db70135068 Bug 1386103 (part 1) - Specify nsAuto[C]String storage size via template parameter. r=dbaron. https://hg.mozilla.org/integration/mozilla-inbound/rev/eec506d87d03103778da654b7755b93a8fad7ae9 Bug 1386103 (part 2) - Convert nsFixed[C]String uses to nsAuto[C]String. r=erahm.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/81fbe7dc05a6 for the incomprehensible muddle of https://treeherder.mozilla.org/logviewer.html#?job_id=121873730&repo=mozilla-inbound on Android x86.

> This is shrinking from 128 -> 64, I know you thought it was arbitrary but is > it possible to dump the string sizes on startup and confirm? I started and opened several sites and didn't hit these code paths.

https://hg.mozilla.org/integration/mozilla-inbound/rev/07241c0f20c10733574d4b71b67d2aa2d643d581 Bug 1386103 (part 1, attempt 2) - Specify nsAuto[C]String storage size via template parameter. r=dbaron. https://hg.mozilla.org/integration/mozilla-inbound/rev/9031387c7ef1fe1db4b68495d1527b56e464c817 Bug 1386103 (part 2, attempt 2) - Convert nsFixed[C]String uses to nsAuto[C]String. r=erahm.

https://hg.mozilla.org/mozilla-central/rev/07241c0f20c1 https://hg.mozilla.org/mozilla-central/rev/9031387c7ef1

Hmm, the changesets you landed are empty :-(

(In reply to Jorg K (GMT+2) from comment #17) > Hmm, the changesets you landed are empty :-( They are! Thanks for pointing this out :)

The Android x86 opt problem is in part 1. It didn't show up when I did my original try push, but something happened between then and my landing attempt. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f6d5b14c51c52a184a9010c716b601e36c9974 is a try job that shows the build failure. There are two very similar errors, both of which are multi-KiB C++ template monstrosities involving NDK headers, NDK types and mozilla::ContainerState::SetupMaskLayerForCSSMask. The Mozilla code in question looks like this: > RefPtr<ImageLayer> maskLayer = > CreateOrRecycleMaskImageLayerFor(MaskLayerKey(aLayer, Nothing()), > [](Layer* aMaskLayer) > { > aMaskLayer->SetUserData(&gCSSMaskLayerUserData, > new CSSMaskLayerUserData()); > } > ); I see no connection between any of this and my nsAuto[C]String changes :(

dvander: this bug is about nsAutoString. Somehow the changes in part 1 are causing an indecipherable and seemingly unrelated compile error in FrameLayerBuilder.cpp on Android 4.2 x86 opt. (See comment 19 for more details.) It smells like a compiler bug. Using function pointers instead of lambdas is enough to quell it, so that's what this patch does.

dvander: 1 week review ping!

(fyi: dvander's on PTO for another week)

Comment on attachment 8895678 [details] [diff] [review] (part 0) - Don't pass lambdas to CreateOrRecycledMaskImageLayerFor Review of attachment 8895678 [details] [diff] [review]: ----------------------------------------------------------------- kats: thank you for the info. Would you mind taking over? It's a very small patch :)

/cc Botond as well as he might be interested in the compiler bug and aforementioned C++ template monstrosities.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0bdc6c0e7ef5fdb86e8fd3fb276b2c9cab364ad Bug 1386103 (part 0) - Don't pass lambdas to CreateOrRecycledMaskImageLayerFor. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/14de940bb317a497f50f6e80a1b5439fe9d049a6 Bug 1386103 (part 1, attempt 3) - Specify nsAuto[C]String storage size via template parameter. r=dbaron. https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f0bad7a33adbe4a0802a65977069dec6a002d5 Bug 1386103 (part 2, attempt 3) - Convert nsFixed[C]String uses to nsAuto[C]String. r=erahm.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24) > /cc Botond as well as he might be interested in the compiler bug and > aforementioned C++ template monstrosities. We've seen errors exactly like this in bug 1367798. There, too, they were triggered by unrelated changes to the codebase. I provided some analysis of the errors in bug 1367798 comment 7 and bug 1367798 comment 9. My conclusion was that, unlikely as it seems, we appear to be running into some sort of undefined behaviour in the compiler. Having this issue crop up in yet another unrelated scenario strengthens that hypothesis. So, Nick, I think you're right about this being a compiler bug, and your workaround is appropriate.

Thank you for the extra info, Botond.

https://hg.mozilla.org/mozilla-central/rev/b0bdc6c0e7ef https://hg.mozilla.org/mozilla-central/rev/14de940bb317 https://hg.mozilla.org/mozilla-central/rev/a5f0bad7a33a