New build warning: { 0:27.47 In file included from /mozilla/js/src/vm/String.h:17:0, 0:27.47 from /mozilla/js/src/vm/PropertyKey.cpp:14: 0:27.47 Warning: enabled by default in /mozilla/js/src/jsatom.h: inline function 'JSAtom* js::ToAtom(JSContext*, const JS::Value&) [with js::AllowGC allowGC = (js::AllowGC)1u; JSContext = JSContext]' used but never defined 0:27.47 /mozilla/js/src/jsatom.h:240:1: warning: inline function 'JSAtom* js::ToAtom(JSContext*, const JS::Value&) [with js::AllowGC allowGC = (js::AllowGC)1u; JSContext = JSContext]' used but never defined [enabled by default] } Looks like PropertyKey.cpp needs to include jsatominlines.h, to pick up that function definition.

...and it can't include that without also including jsinferinlines.h. (otherwise, I get errors like "jsatominlines.h:179:38: error: invalid use of incomplete type ‘JSAtomState {aka struct JSAtomState}’" If I include both headers, then all is well. (I'm pseudo-cribbing from https://mxr.mozilla.org/mozilla-central/source/js/src/jsdate.cpp?mark=50-51#44 fwiw.)

Comment on attachment 710843 [details] [diff] [review] fix Review of attachment 710843 [details] [diff] [review]: ----------------------------------------------------------------- I want warnings as errors. :-( ::: js/src/vm/PropertyKey.cpp @@ +13,5 @@ > #include "js/Value.h" > #include "vm/String.h" > > +#include "jsinferinlines.h" > +#include "jsatominlines.h" These should be alphabetical, and they should be in a section above the non-mozilla */*.h #includes: #include "mozilla/Assertions.h" #include "jsatominlines.h" #include "jsinferinlines.h" #include "gc/Root.h"

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) > > +#include "jsinferinlines.h" > > +#include "jsatominlines.h" > These should be alphabetical, and they should be in a section above the > non-mozilla */*.h #includes: Alphabetical doesn't work -- if I put jsatominlines first, then it hits a bunch of build errors like the one I quoted in comment 1, for incomplete type. I'll move the two new lines up to just before "gc/Root.h" as you suggested, though.

Are you sure? I always thought "xxxinlines.h" were before "-inl.h", which are at the end. https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Includes seems to agree with this.

(Tom's suggested inlines-at-the-end definitely matches the code that I cribbed from, linked in comment 1...)

...er, Tom's right, I didn't think through the inlines part. So the original patch, with the two lines switched, is right.

If the two-lines-switched ordering is wrong, run with what you have for now. That's a bug we should fix, tho.

Pushed the attached patch as-is (because it won't compile w/ switched ordering, as noted above): https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e06ab7a39d