Closed Bug 885511 Opened 11 years ago Closed 11 years ago

Avoid breaking nondefault build options with IWYU (Include What You Use)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: sfink, Assigned: n.nethercote)

References

(Blocks 1 open bug)

Details

So the recent IWYU-based #include pruning ended up breaking at least JS_MORE_DETERMINISTIC and JSGC_GENERATIONAL. Rather than periodically re-fixing these whenever we do an IWYU pass, is there a more general solution? One option would be to just pick compile options for IWYU that will compile as much code as possible. Obviously, this has holes (eg a number of code chunks are either/or), but might be good enough. A different way of accomplishing the same thing would be to add #ifdef IWYU || ... everywhere (or #ifdef COMPILE_ALL_THE_THINGS), to manually label stuff that's necessary, but that seems... well, kind of horrible. I don't know how IWYU works, but can it gather required includes from multiple passes with different #define settings? Or if it's just looking at the code, can it assume all #ifdef and #ifndef are true, and just look at everything? See bug 885502 for the 2nd installement of the JS_MORE_DETERMINISTIC breakage.
Assignee: general → n.nethercote
> See bug 885502 for the 2nd installement of the JS_MORE_DETERMINISTIC > breakage. And it came without warning too, unfortunately, I realised this after updating on my end and saw that the builds were busted.
IWYU is a clang plug-in, so it's seeing the post-CPP code, so there's no chance it can somehow read all the code. This is a general problem with having lots of different builds and not having them all tested on test machines. It's just that IWYU clean-ups are likely to hit this problem more than most changes. I'll do deterministic builds for future IWYU patches, since that's hit a problem. The only preventive measure I can think of is to use #ifndefs to document when a header is only needed for a particular build. For example, in jsiter.cpp: #ifdef JS_MORE_DETERMINISTIC #include "ds/Sort.h" #endif
You could also throw the relevant code behind templates: enum Determinism { Deterministic = true, UnspecifiedDeterminism = false }; template<Determinism IsDeterministic> struct ComputeAsmJSLoadTime; template<> struct ComputeAsmJSLoadTime<Deterministic> { size_t loadTime() { ... }; }; template<> struct ComputeAsmJSLoadTime<UnspecifiedDeterminism> { size_t loadTime() { return 0; } }; and use ComputeAsmJSLoadTime<JS_MORE_DETERMINISTIC>::loadTime(). Same for all the other places. That fixes syntax-based issues. It won't fix semantic issues, due to lazy instantiation. But maybe that solves enough problems to be worth the refactoring.
Oh, I guess you could declare both specializations to get both things compiled: template<> struct ComputeAsmJSLoadTime<Deterministic>; template<> struct ComputeAsmJSLoadTime<UnspecifiedDeterminism>; Of course this is all moderately hackish, but so's anything else that comes to mind here. :-)
I've been building with --enable-gcgenerational and --enable-more-deterministic when doing my recent IWYU work. I think that's enough. Doesn't seem worth doing anything more here.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.