[PATCH] D152274: [clang] Don't create import decls without -fmodules

[Sam McCall via Phabricator via cfe-commits]

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