Nicholas Nethercote [inactive] Reporter
	Description • 11 years ago

For example, for vm/Stack.h the guard is |vm_Stack_h_|.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 1 • 11 years ago

This makes all the include guards consistent across js/, excluding js/src/. I used the same form as in DOM, as described by bz in js-engine.internals thread (and implied by Waldo).

One file I didn't touch in this patch is js/jsd/resource.h. This file is supposedly automatically generated by MSVC and already has a generic include guard - changing it seemed like a bad idea, since it might conflict with other instances of the same file. However, I'm not sure this file should be part of the repository in the first place!

Let me know if you agree with the style. I don't think this part will conflict with your IWYU work, but I don't mind rebasing if so.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 2 • 11 years ago

Fixes up the first part of js/src/ - the base directory, and all directories with only a few files in them.

I added an include guard to js/src/config/gcc_hidden.h because I didn't think it could hurt, but it's a bit of an oddball. I figured if something includes this, and it's included again a few levels deeper, it would just be redundant anyway.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 3 • 11 years ago

This makes the include guards consistent in js/src/assembler/, js/src/builtin/, js/src/ctypes/ and js/src/frontend/.

js/src/assembler/assembler/ARMv7Assembler.h had the same include guard as ARMAssembler.h - I'd be surprised if anything relies on it though, probably just copy-pasta.

All the stuff in js/src/ctypes/libffi is probably pulled in from another repository; I changed them anyway but I can take those out if it's something we periodically sync with.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 4 • 11 years ago

This part fixes up js/src/gc/, js/src/vm/ and js/src/yarr/. Yarr is another place where I'm not sure I should be making changes. I think js/src/yarr/wtfbridge.h is ours, at least.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 5 • 11 years ago

And last but not least, this part fixes up js/src/ion/.

Note that, in passing, all these patches also fix up files that ended with more than one line feed.

	Nicholas Nethercote [inactive] Reporter
	Comment 6 • 11 years ago

Thanks for diving into this!

I assigned this to myself and marked is at dependent on bug 634839 because I was worried about conflicts, but maybe it won't be a problem, and since you've got this far I don't want to block you.

I've done a quick skim of your patches.  Here are the things I've found.

- Don't bother with js/jsd/.  It's headed for the chopping block, and isn't worth the effort.

- Don't bother with js/xpconnect/.  Even though it's under js/, it's not part of SpiderMonkey, so the rules are different.

- Don't do js/src/ctypes/libffi.

- Don't do js/src/config/gcc_hidden.h.

- Distinguishing ARMAssembler.h and ARMv7Assembler.h seems reasonable.

- Changing js/src/yarr should be ok -- looking at |hg log| we modify those.  If it gets overwritten in the future due to a Yarr import it won't matter.

- In the JS internals list we agreed that the guard names should end with '_', which you haven't done.  But I can live with no underscore, so I've emailed the list for consent.

- The guard names you've used have the full paths from the base of the Mozilla source tree, but we only want them from the base of the SpiderMonkey source tree, i.e. within js/src/.  That way they mirror the paths used when we #include them.

  So where you have |js_src_jsfoo_h| you should just have |jsfoo_h|, and |js_src_foo_Bar_h| should just be |foo_Bar_h|.

  And for the ones in js/public/, use the form |js_Foo_h|, rather than |js_public_Foo_h|.  This mirrors the |#include "js/Foo.h"| form.

I think that's everything!  I'll clear the f? flags in preparation for round 2.  Thanks.

	Jeff Walden [:Waldo]
	Comment 7 • 11 years ago

(In reply to Nicholas Nethercote [:njn] from comment #6)
> - The guard names you've used have the full paths from the base of the
> Mozilla source tree, but we only want them from the base of the SpiderMonkey
> source tree, i.e. within js/src/.  That way they mirror the paths used when
> we #include them.
> 
>   So where you have |js_src_jsfoo_h| you should just have |jsfoo_h|, and
> |js_src_foo_Bar_h| should just be |foo_Bar_h|.
> 
>   And for the ones in js/public/, use the form |js_Foo_h|, rather than
> |js_public_Foo_h|.  This mirrors the |#include "js/Foo.h"| form.

There's some mailing-list/newsgroup mirroring snafuing here -- I subsequently argued we should pick a system that Gecko could adopt as well, which I think is what Emanuel was picking up on.  See the thread:

https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.js-engine.internals/Smt0r67E_dE

Sigh, mirroring, sigh, newsgroup haters gonna hate.

	Nicholas Nethercote [inactive] Reporter
	Comment 8 • 11 years ago

> There's some mailing-list/newsgroup mirroring snafuing here

Hmm, I didn't get the last few messages sent to my email.  Grr.

I guess we need Luke to weigh in here, between these three choices:

a) #ifndef jsfoo_h_
b) #ifndef jsfoo_h
c) #ifndef js_src_jsfoo_h

I vote for (b), because I like the guard name mirroring the #include used for the header.  I.e. we write |#include "jsfoo.h"|, not |#include "js/src/jsfoo.h"|.  And consistency with Gecko has never been a goal of SpiderMonkey, so I don't see why we'd start now...

	Nicholas Nethercote [inactive] Reporter
	Comment 9 • 11 years ago

> And consistency with Gecko has never been a goal of
> SpiderMonkey, so I don't see why we'd start now...

Actually, (b) *is* consistent with what bz said:

> New additions to Gecko have been using basically what you'd put in the
> #include string, with '/' and '.' replaced by _. 

Even better! :)

	Jeff Walden [:Waldo]
	Comment 10 • 11 years ago

What's the point of it mirroring the #include?  It's an implementation detail.  Nobody should be using it except the header.  And when the include location changes, there's no reason the file itself should have to change.  Further, having to know the include path is an extra thing to think about for every new file that's created.

One other issue with using include path is that it's not a fixed constant -- it depends where you are in the tree.  That's why source location is a better key for creating an include-guard.  And for embedders, the include path isn't "jsapi.h".  It's "js17/jsapi.h", or similar for the release in question.

	Luke Wagner [:luke]
	Comment 11 • 11 years ago

I vote for (b) for the reasons already given: matches Gecko, it has a simple rule.

The files in js/public always get #included as, e.g., #include "js/Vector.h", so Vector.h would unambiguously use #ifndef js_Vector_h.  The exported files in js/src... well they should stop being exported or be moved to js/public... but for now, they are always #included without any directory so I think it's still unambiguous to write, e.g., #ifndef jsapi_h.  All of this is to say that I don't see any real problem that needs to be solved by an additional js_src_* prefix.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 12 • 11 years ago

Thanks for the feedback. I can make mass changes pretty easily at this point, so don't worry about that. 

(In reply to Nicholas Nethercote [:njn] from comment #6)
> - Don't bother with js/jsd/.  It's headed for the chopping block, and isn't
> worth the effort.
This one wasn't exactly hard, but I can easily take it out.

> - Don't bother with js/xpconnect/.  Even though it's under js/, it's not
> part of SpiderMonkey, so the rules are different.
Something here actually lead to linking errors, so I'm happy to drop this.

> - Don't do js/src/ctypes/libffi.
Yeah, those were odd. In addition I had to undo the changes to js/src/ctypes/typedefs.h since that file is legitimately included several times, and only meant for local use.

> - Don't do js/src/config/gcc_hidden.h.
Yeah, turns out files in js/src/config have to match the ones in ./config anyway.

As for whether or not to mirror include style: in theory, including the directory structure is easier since it requires no knowledge of include rules. An automated script could keep the include guards up-to-date without any knowledge of how files are included from elsewhere in the tree.

On the other hand, it seems like an easy enough distinction to make here: leave out js_src_ for stuff in js/src/, but otherwise include the js_ part. Things in js/src/ are pretty consistent about prefixing their names with 'js', so that should be fine. Then there's headers in e.g. js/src/ion/arm/ which all append -arm, so the include guard ends up looking pretty redundant. I'm not sure which _arm to leave out though :)

	Nicholas Nethercote [inactive] Reporter
	Comment 13 • 11 years ago

> On the other hand, it seems like an easy enough distinction to make here:
> leave out js_src_ for stuff in js/src/, but otherwise include the js_ part.

I don't follow... if you remove the js/jsd/ and js/xpconnect/ changes, aren't *all* the remaining changes within js/src/ or one of its children directories?  (Or perhaps you're excluding the children directories in your statement?)


> Things in js/src/ are pretty consistent about prefixing their names with
> 'js', so that should be fine. Then there's headers in e.g. js/src/ion/arm/
> which all append -arm, so the include guard ends up looking pretty
> redundant. I'm not sure which _arm to leave out though :)

Don't leave either out.  E.g. ion_arm_Assembler_arm_h is fine.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 14 • 11 years ago

(In reply to Nicholas Nethercote [:njn] from comment #13)
> I don't follow... if you remove the js/jsd/ and js/xpconnect/ changes,
> aren't *all* the remaining changes within js/src/ or one of its children
> directories?  (Or perhaps you're excluding the children directories in your
> statement?)
There's still js/ductwork/debugger/JSDebugger.h, 5 files in js/ipc/ and 16 in js/public/.

	Nicholas Nethercote [inactive] Reporter
	Comment 15 • 11 years ago

> There's still js/ductwork/debugger/JSDebugger.h, 5 files in js/ipc/ and 16
> in js/public/.

Ah.  So, for js/public/, use |js_Foo_h| as we discussed.

For JSDebugger.h... probably |JSDebugger_h|, since that's how its #included in JSDebugger.cpp.

And for the ipc ones, probably e.g. |mozilla_jsipc_ObjectWrapperParent_h|, again to mirror #includes.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 16 • 11 years ago

No longer touches js/dbg/ and js/xpconnect/, and applies feedback from comment #15.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 17 • 11 years ago

Changed to the agreed style (no js_src_ prefix) and no longer touches js/src/config/.

This also moves the include guard in jsinferinlines.h back to its original location. By moving it to the top, I got compile errors in jsobjinlines.h. While I'm sure those can be fixed (probably with some more IWYU), it didn't feel appropriate for an otherwise mechanical change. That does mean that GCC's Multiple-Include Optimization won't apply on this file for now.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 18 • 11 years ago

Changed to the agreed style (no js_src_ prefix) and leaves ctypes/libffi/ alone. 

Compared to version 1, this also reverts the changes to ctypes/typedefs.h, since the file is legitimately included several times and only used locally.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 19 • 11 years ago

Changed to the agreed style (no js_src_ prefix), and splits out the changes to yarr/ so they can be more easily reverted if necessary.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 20 • 11 years ago

Changed to the agreed style (no js_src_ prefix). Split off from part 4a to make it easier to revert later if needed.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 21 • 11 years ago

Changed to the agreed style (no js_src_ prefix); no other changes from v1.

Oh, in comment #16 I meant js/jsd/, not js/dbg/.

	Nicholas Nethercote [inactive] Reporter
	Comment 22 • 11 years ago

> This also moves the include guard in jsinferinlines.h back to its original
> location. By moving it to the top, I got compile errors in jsobjinlines.h.
> While I'm sure those can be fixed (probably with some more IWYU), it didn't
> feel appropriate for an otherwise mechanical change.

Good idea.  That file's a mess and you're right to not worry about it here.

	Nicholas Nethercote [inactive] Reporter
	Comment 23 • 11 years ago

Comment on attachment 764770 [details] [diff] [review]
Part 1 v2 - Make include guards consistent in js/ductwork/, js/ipc/ and js/public/

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

::: js/ductwork/debugger/JSDebugger.h
@@ +26,5 @@
>  
>  }
>  }
>  
> +#endif /* JSDebugger_h */

I prefer C++ style comments, but I'm not going to make you change all of these :)

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 24 • 11 years ago

(In reply to Nicholas Nethercote [:njn] from comment #22)
> Good idea.  That file's a mess and you're right to not worry about it here.

I did do a little bit of digging: the problem is the vm/Stack-inl.h include. Everything else can be moved below the include guard, but that one is part of a chain that can't be broken without breaking compilation. Without it, jsobjinlines.h and ObjectImpl-inl.h somehow forget what's in jsinferinlines.h, and since the include guard is set, they can't pull it in again. I don't understand how that happens though.

	Nicholas Nethercote [inactive] Reporter
	Comment 25 • 11 years ago

Comment on attachment 764776 [details] [diff] [review]
Part 2 v2 - Make include guards consistent in some of js/src/

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

BTW, *please* try server these patches before landing! :)

::: js/src/jsapi-tests/tests.h
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef jsapi_tests_tests_h
> +#define jsapi_tests_tests_h

Interesting to see how many of these headers lacked #ifndef wrappers.  Thanks for fixing them all.

::: js/src/jsdhash.h
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef jsdhash_h
> +#define jsdhash_h

I'm in the process of removing this file in bug 783820.  Let's see who lands first! :)

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 26 • 11 years ago

(In reply to Nicholas Nethercote [:njn] from comment #25)
> BTW, *please* try server these patches before landing! :)
I was going to ask you about that - I don't have access myself! With all the files this touches, it does seem like a good idea.

	Nicholas Nethercote [inactive] Reporter
	Comment 27 • 11 years ago

Comment on attachment 764782 [details] [diff] [review]
Part 3 v2 - Make include guards consistent in some more of js/src/

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

It might be worth putting a comment at the top of ctypes/typedefs.h, such as "This file deliberately does not have a #ifndef wrapper because it is #included multiple times in ctypes/CTypes.h."

	Nicholas Nethercote [inactive] Reporter
	Comment 28 • 11 years ago

Comment on attachment 764785 [details] [diff] [review]
Part 4a v2 - Make include guards consistent in still more of js/src/

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

Hot damn we have a lot of header files.

	Nicholas Nethercote [inactive] Reporter
	Comment 29 • 11 years ago

Comment on attachment 764787 [details] [diff] [review]
Part 4b v2 - Make include guards consistent in js/src/yarr/

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

::: js/src/yarr/YarrCanonicalizeUCS2.h
@@ +25,5 @@
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
>   */
>  
> +#ifndef yarr_YarrCanonicalizeUCS2_h
> +#define yarr_YarrCanonicalizeUCS2_h

This is an odd file -- there's a script yarr/YarrCanoncializeUCS2.js that generates it, and yet we have a copy in the repository.  

I'm not sure what's really supposed to happen, so what I've done in other bugs is modify the script as well.  Can you do that here?  Thanks.

	Nicholas Nethercote [inactive] Reporter
	Comment 30 • 11 years ago

Comment on attachment 764790 [details] [diff] [review]
Part 5 v2 - Make include guards consistent in js/src/ion/

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

Lovely!  Thanks again for doing this.

	Nicholas Nethercote [inactive] Reporter
	Comment 31 • 11 years ago

> I was going to ask you about that - I don't have access myself! With all the
> files this touches, it does seem like a good idea.

Ok, I can take over from here.  Don't worry about addressing my handful of nits, I can do that myself.  I'll try server and land them.

> I did do a little bit of digging: the problem is the vm/Stack-inl.h include.
> Everything else can be moved below the include guard, but that one is part
> of a chain that can't be broken without breaking compilation. Without it,
> jsobjinlines.h and ObjectImpl-inl.h somehow forget what's in
> jsinferinlines.h, and since the include guard is set, they can't pull it in
> again. I don't understand how that happens though.

There are cyclic dependencies.  jorendorff wrote a script that finds them, it says the following:

js/src/jsobjinlines.h
 -> js/src/jsfuninlines.h
     -> js/src/vm/ScopeObject-inl.h
         -> js/src/jsinferinlines.h
             -> js/src/vm/Stack-inl.h
                 -> js/src/vm/ScopeObject-inl.h
                 -> js/src/ion/BaselineFrame-inl.h
                     -> js/src/vm/ScopeObject-inl.h
                 -> js/src/jsscriptinlines.h
                     -> js/src/vm/Shape-inl.h
                         -> js/src/vm/ScopeObject-inl.h
         -> js/src/jsscriptinlines.h
         -> js/src/jsobjinlines.h

We need to break the cycle... but in another bug.

Thanks again for doing this!  It's a huge help.

	Emanuel Hoogeveen [:ehoogeveen] Assignee
	Comment 32 • 11 years ago

(In reply to Nicholas Nethercote [:njn] from comment #29)
> This is an odd file -- there's a script yarr/YarrCanoncializeUCS2.js that
> generates it, and yet we have a copy in the repository.  
> 
> I'm not sure what's really supposed to happen, so what I've done in other
> bugs is modify the script as well.  Can you do that here?  Thanks.

I think yarr/YarrCanoncializeUCS2.js generates yarr/YarrCanoncializeUCS2.cpp, but the header is either imported or hand-crafted to go along with the script.

> Thanks again for doing this!  It's a huge help.

No problem, thanks for the reviews :) If you want I can upload some rebased patches (including that comment you asked for).

	Nicholas Nethercote [inactive] Reporter
	Comment 33 • 11 years ago

> ::: js/src/yarr/YarrCanonicalizeUCS2.h
> @@ +25,5 @@
> >   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> >   */
> >  
> > +#ifndef yarr_YarrCanonicalizeUCS2_h
> > +#define yarr_YarrCanonicalizeUCS2_h
> 
> This is an odd file -- there's a script yarr/YarrCanoncializeUCS2.js that
> generates it, and yet we have a copy in the repository.  

Oh wait, I'm mixing this up with YarrCanonicalizeUCS2.cpp.  So there's no problem here.

	Nicholas Nethercote [inactive] Reporter
	Comment 34 • 11 years ago

Try server run:
https://tbpl.mozilla.org/?tree=Try&rev=ceec685c6fc0

	Nicholas Nethercote [inactive] Reporter
	Comment 35 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/41caf6414eae
https://hg.mozilla.org/integration/mozilla-inbound/rev/cba55a540e2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/aedf5992770a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa4e688f5daa
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bce31cdb901
https://hg.mozilla.org/integration/mozilla-inbound/rev/618c1cf5a236

\o/

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/41caf6414eae
https://hg.mozilla.org/mozilla-central/rev/cba55a540e2a
https://hg.mozilla.org/mozilla-central/rev/aedf5992770a
https://hg.mozilla.org/mozilla-central/rev/fa4e688f5daa
https://hg.mozilla.org/mozilla-central/rev/4bce31cdb901
https://hg.mozilla.org/mozilla-central/rev/618c1cf5a236