897553 - Create a script to sanitize includes across the tree (and use it)

Status: NEW

(Core :: General, enhancement)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Sanitize Includes script, v1.2

In bug 888088 I created a script to automatically go through and fix the include statements in js/src/. This bug is for further enhancing that script, and looking into applying it to other locations in the tree.

To start out with, the attached version of the script fixes a problem where include ordering could be wrong because calling cmp() on two strings doesn't do a lexicographical compare - the easiest way around this is to use (a > b) - (a < b) instead.

Aside from fixing bugs in the script, additional enhancements I currently want to look into include:
1) Check and fix include guard style (adding an include guard to headers that don't have any)
2) Support comparing and building up sets of assumptions directly instead of through comparing (ordered) parent lists
3) Support fixing specific includes in place (to deal with e.g. js/src/assembler/).
4) Get namespace exports directly from the build system instead of listing them manually
5) Build a list of imported code to specifically exclude (excluding code we patch heavily in-tree)
6) Integrate with the script from bug 880088 to check for include cycles
7) Look into optionally minimizing whitespace-only changes

Note to self: Generated changes to js/src/prmjtime.cpp look broken with this version.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Sanitize Includes script, v1.3

This version fixes the problems with js/src/prmjtime.cpp (and possibly other places), and only shows or writes out changes when they're not all white space. This was easy to do and might help review some of this stuff when the time comes; I'm not confident enough in the ability of the script to not mess up white space to leave these otherwise empty diffs in there.

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 780620 [details]
Sanitize Includes script, v1.3

Setting these to text/plain so you can actually look at them in the bug.

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Sanitize Includes script, v1.4

This version removes partitions with only one include in them so it doesn't end up messing with the whitespace around them, and adds js/src/assembler/wtf/Platform.h to a new list of headers to leave alone, which reduces the suggested changes to js/src/assembler/ by quite a bit. However, compilation with the suggested output still fails (some include cycle for mozilla/Assertions.h, it seems).

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Sanitize Includes script, v1.5

This version makes the check for (inlines.h, -inl.h) headers case-insensitive, and does some refactoring in that area to make it less horrible. It also fixes a bug introduced in version 1.3 where it would try to delete a line at location -1 (the default for generated include lines), thereby deleting the last line in the file (oops).

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Sanitize Includes script, v1.6

This version contains two changes:
1) #includes will now be ordered such that more nested paths appear later. For instance:
...
#include "ion/IonSpewer.h"
#include "ion/VMFunctions.h"
#include "ion/arm/BaselineHelpers-arm.h"
...

2) #elif and #else statements following another #elif statement that was moved elsewhere will now be fixed with the appropriate conditions. For instance:
...
+#if defined(JS_CPU_ARM) && !defined(JS_CPU_X64) && !defined(JS_CPU_X86)
+# include "ion/arm/Lowering-arm.h"
+#endif
+
+#if defined(JS_CPU_X64) && !defined(JS_CPU_X86)
+# include "ion/x64/Lowering-x64.h"
+#endif
+
 #if defined(JS_CPU_X86)
 # include "ion/x86/Lowering-x86.h"
-#elif defined(JS_CPU_X64)
-# include "ion/x64/Lowering-x64.h"
-#elif defined(JS_CPU_ARM)
-# include "ion/arm/Lowering-arm.h"
-#else
+#elif !defined(JS_CPU_ARM) && !defined(JS_CPU_X64)
 # error "CPU!"
 #endif
...
Yes, I know that this is still ugly. I'll probably introduce a check at some point to leave includes out of order if they're in the same group and changing the order would require generating new preprocessor statements. Fixing the correctness issue had a higher priority though. Of course, you could also fix this up manually to be:
...
#if defined(JS_CPU_ARM)
# include "ion/arm/Lowering-arm.h"
#elif defined(JS_CPU_X64)
# include "ion/x64/Lowering-x64.h"
#elif defined(JS_CPU_X86)
# include "ion/x86/Lowering-x86.h"
#else
# error "CPU!"
#endif
...
to make the current script happy.

These were the last known bugs in the script, but I should really start working on some self-tests when I have time, to make sure things don't regress.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.