1225745 - Rename nsAuto{,C}String to nsStack{,C}String

Status: NEW

(Core :: XPCOM, defect)

Description

[Mike Hommey [:glandium]]

One reason why nsAuto{,C}String are poorly used (cf. bug 1225682 + all the uses as function return value or argument I will file another bug about) are likely caused by the poor naming of the type.

Let's fix this by renaming it.

Comment 1

[Nicholas Nethercote [inactive]]

These docs (and possibly more) will need updating:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings
https://developer.mozilla.org/en-US/docs/Mozilla_external_string_guide
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Can't these uses be prevented via static analysis (MOZ_STACK_CLASS) without roto-tilling the entire tree with a name change?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Are you going to rename every use of Auto in the tree to Stack for stack objects?  Like Mutex::AutoLock, etc?  Otherwise this change is going to make our naming more inconsistent and harder for developers.

Like it or not, we use "Auto" to signal that a class should be on the stack.  You should rename them all or none of them, IMO.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Can't these uses be prevented via static analysis (MOZ_STACK_CLASS) without
> roto-tilling the entire tree with a name change?

That's bug 1225682

(In reply to Ben Kelly [:bkelly] from comment #3)
> Are you going to rename every use of Auto in the tree to Stack for stack
> objects?  Like Mutex::AutoLock, etc?  Otherwise this change is going to make
> our naming more inconsistent and harder for developers.
> 
> Like it or not, we use "Auto" to signal that a class should be on the stack.
> You should rename them all or none of them, IMO.

Auto in those classes has essentially the meaning of "RAII". AutoString has nothing to do with RAII.

Comment 5

[Mike Hommey [:glandium]]

That is, nsAutoString is not any more special for RAII than nsString. The Auto part is that if the string is smaller than 64 characters, it's store inside the nsAutoString object itself instead of on the heap.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

So you're changing nsAutoTArray<> as well?

Comment 7

[Nathan Froyd [:froydnj]]

I'm not sure a complete name change here is worthwhile.

(In reply to Ben Kelly [:bkelly] from comment #6)
> So you're changing nsAutoTArray<> as well?

I think nsAutoTArray should not be renamed (despite the inconsistency), because it at least lets the user control the amount of storage.  nsAuto*String has no such knob...though it's not entirely clear to much just how much space these are wasting...perhaps glandium has some data.

mozilla::Vector has the better design here of letting you just specify an amount of inline storage and dispensing with the "Auto" concept entirely.  I think LLVM's classes use "Small" for things with inline storage (SmallVec, SmallString, etc.).

Comment 8

[Nicholas Nethercote [inactive]]

> Like it or not, we use "Auto" to signal that a class should be on the stack.

As Mike said, our usage of "Auto" is inconsistent and confusing. In many cases (e.g. AutoLock, nsAutoPtr) it means "this is an RAII type". (MFBT also used "Scoped" for this.) IMO this is the "normal" meaning of "Auto", and isn't too bad, though still potentially confusing.

In the case of nsAutoString, it means "this string has inline chars". IMO this is a terrible use of "Auto", being totally non-obvious. And there are many misuses of nsAutoString it in the code, e.g. for function arguments, which demonstrates that people are misunderstanding what it's meant to be for.

I'm not aware of any cases where "Auto" means "this should be on the stack". (RAII types are often used on the stack, but that's not mandatory.)


> You should rename them all or none of them, IMO.

Getting rid of all confusing uses of "Auto" is a good idea. But I disagree that they need to all be done at once. "Perfect is the enemy of good" and all that.

Comment 9

[Nicholas Nethercote [inactive]]

> I think nsAutoTArray should not be renamed (despite the inconsistency),
> because it at least lets the user control the amount of storage. 
> nsAuto*String has no such knob...though it's not entirely clear to much just
> how much space these are wasting...perhaps glandium has some data.

The "control the amount of storage" part is entirely orthogonal. As I said in comment 8, in our codebase "Auto" mostly means "RAII type". Having it mean something else for some types is horribly confusing.

(I fully admit to only just now fully understanding what nsAutoString and nsAutoTArray are. I always vaguely assumed they were RAII types somehow -- though I'm not even sure what that would mean for strings and arrays -- and I never really looked closely enough at them to know for sure.)

Comment 10

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> I'm not sure a complete name change here is worthwhile.

We don't run static analysis on local builds. You can be sure that keeping the ambiguous name will lead people to burn static analysis when landing their changes.

(In reply to Nathan Froyd [:froydnj] from comment #7)
> I'm not sure a complete name change here is worthwhile.
> 
> (In reply to Ben Kelly [:bkelly] from comment #6)
> > So you're changing nsAutoTArray<> as well?
> 
> I think nsAutoTArray should not be renamed (despite the inconsistency),
> because it at least lets the user control the amount of storage. 
> nsAuto*String has no such knob...though it's not entirely clear to much just
> how much space these are wasting...perhaps glandium has some data.

From a quick test it looks like it's a few dozen kilobytes of waste for a small session. Nothing too catastrophic, but that doesn't make it a justification to keep the current status quo.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Rename nsAuto{,C}String to nsStack{,C}String

This is how I created the patch:

git grep -l nsAutoString | xargs sed -i s/nsAutoString/nsStackString/g
git grep -l nsAutoCString | xargs sed -i s/nsAutoCString/nsStackCString/g
git grep -l nsTAutoString | xargs sed -i s/nsTAutoString/nsTStackString/g

Then I adjusted xpcom/string/string-template-def-char.h and xpcom/string/string-template-def-unichar.h for spaces.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Rename nsAuto{,C}String to nsStack{,C}String

I don't know why, but the previous one wasn't on top of the patch that adds MOZ_STACK_CLASS to nsTAutoString_CharT for some reason.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8689854 [details] [diff] [review]
Rename nsAuto{,C}String to nsStack{,C}String

Review of attachment 8689854 [details] [diff] [review]:
-----------------------------------------------------------------

Given comments in the dependent bug, I'm not so sure this patch is going to fly.