Closed Bug 838753 Opened 7 years ago Closed 7 years ago

PropertyKey.cpp triggers "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]"

Categories

(Core :: JavaScript Engine, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.)
Attached patch fixSplinter Review
Attachment #710843 - Flags: review?(jwalden+bmo)
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"
Attachment #710843 - Flags: review?(jwalden+bmo) → review+
(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
Flags: in-testsuite-
Duplicate of this bug: 838553
https://hg.mozilla.org/mozilla-central/rev/c8e06ab7a39d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.