-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
<xkeycheck.h>: Fix circular inclusion for Header Units #1692
Conversation
@@ -6,7 +6,10 @@ | |||
#pragma once | |||
#ifndef _XKEYCHECK_H | |||
#define _XKEYCHECK_H | |||
#include <yvals_core.h> | |||
|
|||
// xkeycheck.h assumes that it's being included by yvals_core.h in a specific order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of assuming, should we #error
if _YVALS_CORE_H_
isn't defined? I'm inclined to not check, since (1) as you say elsewhere no user should ever include this and there's no reason to include from elsewhere in the STL, and (2) if someone does somehow manage to include this before yvals_core.h
the effect will simply be to disable the contents (_STL_COMPILER_PREPROCESSOR
won't be defined). There's no danger of silent bad codegen.
The flip side of this coin is, if this file is only supposed to be included from one place ever, why not inline its contents and delete this file instead of forcing the preprocessor to inline it into yvals_core.h
on every compile?
(This is an "approve with suggestions" comment: it's worth discussion, but I certainly won't be bothered if we merge this as is.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to not check
Same. I would add (3) this wouldn't enforce the particular ordering we care about (early enough to protect most of yvals_core.h
from macros, but after the definitions of _STL_COMPILER_PREPROCESSOR
and _HAS_CXXmeow
).
The flip side of this coin is, if this file is only supposed to be included from one place ever, why not inline its contents and delete this file instead of forcing the preprocessor to inline it into yvals_core.h on every compile?
We could do that (as we recently did for <cmath>
), but xkeycheck.h
is rather large (almost 600 lines, 20 KB) and extremely boring (it should do nothing for builds that aren't doomed); yvals_core.h
is already big enough (almost 1400 lines, 67 KB) and contains lots of important stuff.
What I really want is for the compilers to reward keyword macros with doom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good (xkeycheck sounds like an NSA program doesn't it).
Works towards #60.
Discovered by @olgaark - we've done something weird in the machinery that detects macroized keywords, and this is a problem for building Standard Library Header Units.
We had correctly marked
xkeycheck.h
as not being automatically importable as a header unit:STL/stl/inc/header-units.json
Line 126 in 3c13e3e
And correctly marked
yvals_core.h
(the STL's central internal header) as being importable (like the vast majority of STL headers):STL/stl/inc/header-units.json
Line 148 in 3c13e3e
Next,
yvals_core.h
defines_STL_COMPILER_PREPROCESSOR
:STL/stl/inc/yvals_core.h
Lines 9 to 18 in 3c13e3e
And includes
vcruntime.h
for_HAS_CXX17
etc. before includingxkeycheck.h
:STL/stl/inc/yvals_core.h
Lines 341 to 342 in 3c13e3e
🪲 Finally, because
xkeycheck.h
wants_STL_COMPILER_PREPROCESSOR
and_HAS_CXX17
etc. it includesyvals_core.h
, which is circular inclusion!STL/stl/inc/xkeycheck.h
Lines 6 to 10 in 3c13e3e
This wasn't causing problems for ordinary compilation (because
yvals_core.h
's#pragma once
and include guards prevent the recursion), but attempting to build STL Header Units in an optimal way involves "source dependencies" scanning, and this makesyvals_core.h
look like it includes itself, which is a problem.The fix is simple: remove the circular inclusion, and simply comment that
xkeycheck.h
is super special. Nothing else in the STL includes it, and users should not be including it directly. (I vaguely recall that at one time, every STL header tried to includexkeycheck.h
itself, but we centralized this long ago.)