Closed Bug 899415 Opened 6 years ago Closed 6 years ago

OdinMonkey: better split out .h/.cpp files

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file)

This is just preparatory cleanup for further additions.  AsmJS.h contains declarations that should be in AsmJSLink.h/AsmJSSignalHandlers.h; AsmJS.cpp contains definitions that should be in AsmJSModule.cpp.  This patch creates these files and minimizes #include bootlegging.
Attachment #782920 - Flags: review?(bbouvier)
Comment on attachment 782920 [details] [diff] [review]
collect-asmjsmodule

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

Nice, it should enhance clarity. It compiles even with --enable-perf and doesn't generate any (or should I say 'more') warning with clang++ \o/

Have you used IWYU?

::: js/src/ion/AsmJSLink.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> +#include "ion/AsmJSLink.h"

Could it be possible to move this include near the other ion/ includes too, please?

::: js/src/ion/AsmJSSignalHandlers.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> +#include "ion/AsmJSSignalHandlers.h"

It'd be nice to have this include near the other ion-related one (ion/AsmJSModule.h).
Attachment #782920 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Have you used IWYU?

Not directly, no.

> > +#include "ion/AsmJSLink.h"
> 
> Could it be possible to move this include near the other ion/ includes too,
> please?

In general you'd be right, but a couple months ago we agreed (on mozilla.dev.tech.js-engine-internals) on the following changes to the #include order:
  https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style#.23include_ordering
whereby the first #include of X.cpp is always X.h.  The benefit of this is that you know that X.h is self-contained (it doesn't compile just because of some preceding #include).
(In reply to Luke Wagner [:luke] from comment #2)
> In general you'd be right, but a couple months ago we agreed (on
> mozilla.dev.tech.js-engine-internals) on the following changes to the
> #include order:

Doh! Thanks for the reminder :)
https://hg.mozilla.org/mozilla-central/rev/6c88cddc6d89
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.