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)
Core
JavaScript Engine
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.
Reporter | ||
Updated•11 years ago
|
Assignee: general → n.nethercote
Comment 1•11 years ago
|
||
> 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.
Assignee | ||
Comment 2•11 years ago
|
||
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
Comment 3•11 years ago
|
||
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.
Comment 4•11 years ago
|
||
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. :-)
Assignee | ||
Comment 5•11 years ago
|
||
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.
Description
•