sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
The internal state when modules are off but layering check is on is really
counterintuitive, and clearly not all configurations have been tested :-(
I'm pretty sure this is the right behavior, though.
Just readability nits.
================
Comment at: clang/lib/Sema/SemaModule.cpp:639
- bool ShouldAddImport = !IsInModuleIncludes;
+ bool ShouldAddImport = !IsInModuleIncludes && getLangOpts().Modules;
----------------
It's been a couple of weeks since we looked at this together, and I already had
trouble following the logic here.
I think it would be slightly clearer by:
- flipping the order (LangOpts.Modules is "more fundanmental"/a prerequisite)
- moving this var *after* the comment that explains it
- adding a brief comment for the extra clause.
Something like
```
// If we are really importing a module (not just checking layering)
// due to an #include in the main file, synthesize an ImportDecl.
bool ShouldAddImport = getLangOpts().Modules && !IsInModuleIncludes;
if (ShouldAddImport) {
```
I'd also consider inlining+dropping the `ShouldAddImport` variable, since the
comment explains pretty well what the condition represents
all up to you though
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152274/new/
https://reviews.llvm.org/D152274
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
