Closed Bug 765990 Opened 10 years ago Closed 10 years ago

Never use Rooted as a temporary, only as a local variable


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)


(Whiteboard: [js:t])


(1 file)

Attached patch PatchSplinter Review
Using it as a temporary introduces various conversion ambiguities that requires an |operator Handle<T>| to placate clang.  It's unclear if this is a spec thing or a clang quirk, but either way it doesn't really matter: there's extra complexity to handle.  The extra conversion operator also introduces problems because it can make conversion ambiguous, in certain circumstances.

Another problem with using it as a temporary is that, used in concert with a local variable, you get actually wrong behavior -- see bug 759895.

In sum, we should only use Rooted as a local variable, not as a temporary.  Here's a patch implementing this.  It tends to bitrot pretty easily, although generally not too horribly each time, so it'd be good to get it reviewed reasonably soon.
Attachment #634257 - Flags: review?(luke)
I don't buy this argument.  Re: bug 759895, if anyone writes code like that there will be an assert botch whenever that code executes, which seems pretty minor to me.  The fact that clang requires an extra one line method to make it happy doesn't seem that relevant.  And these new variables are adding a ton of cruft to the code and make it much harder for me to read than when the Rooted<X> are constructed as temporaries.
I think this is a good patch.  We have two ambiguous ways to go from a Rooted to a Handle; it is no surprise there is an ambiguity and I'm surprised GCC accepts it.

Stylistically, I'm happy to see all these RootedVars be given their own statements, not just b/c of the LIFO hazard, but because they make the callsites harder to read.  Apparently the IM folks have already adopted the style of having separate declarations for Rooted so this patch would bring more uniformity to SM.

cc'ing more people who may wish to state their preference.
Whiteboard: [js:t]
Comment on attachment 634257 [details] [diff] [review]

Given that IM is already doing this and it seems to avoid never-ending clang headaches, I don't see any reason not to do this.

r+ with two changes, already discussed:

Could you use the .tbl to auto-generate something that returns atom handles instead of using fromMarkedLocation?

Could you remove the 'const' from the Handle(Rooted &) constructor so that we can catch accidental use of temporaries in GCC?
Attachment #634257 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #3)
> Could you use the .tbl to auto-generate something that returns atom handles
> instead of using fromMarkedLocation?

Or just use Rooted, your choice.
The Rooted class should also have JS_GUARD_OBJECT_NOTIFIER, which will I believe give a compile error somewhere other than clang if someone uses Rooted as a temporary in the future.
I used Rooted for now for the atomState atom uses.  The .tbl thing is something we should do, but it seems non-trivial and tangential enough to warrant its own patch and its own bug.

I also removed const as mentioned, which will also address comment 5's concern (temporaries passed to non-const ref parameters are a very well-known, widely-implemented compile error, no need to double down on the errors with guard macros).
Target Milestone: --- → mozilla16
Sigh, forgot that building the shell alone wouldn't be enough for this change.  Fixed the couple other users outside the engine, verified it built fine, repushed:
Some compilers are dumb.  Made a tweak, repushed, now it appears to be sticking:
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.