[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In my testing I was able to find a case where we go around `requires` like

  // module.modulemap
  module Main {
    header "main.h"
    export *
  
    extern module A "extra.modulemap"
    requires nonsense {
      extern module B "extra.modulemap"
    }
  }

  // extra.modulemap
  module Main.A {}
  module Main.B {}

In this case we can do `@import Main.A` and `@import Main.B` despite unsatisfied `requires` in the module.modulemap. I'm not sure it is a bug but I'm not really in a position to predict the expectations of other developers, especially those not deeply familiar with module maps.

Other than this example, I haven't found any other strange `requires`/`extern` interactions. For example, attributes like `[extern_c]` are communicated through module/sub-module relationship and not through `extern` parsing location, so block `requires` does not affect the attributes (as far as I can tell).



================
Comment at: clang/lib/Lex/ModuleMap.cpp:2315-2320
+  bool Satisfied = true;
+  for (const RequiresFeature &F : RFL) {
+    if (Module::hasFeature(F.Feature, Map.LangOpts, *Map.Target) !=
+        F.RequiredState)
+      Satisfied = false;
+  }
----------------
You can try using `all_of` for this computation. But I don't know without trying if it would improve readability, to be honest. If it looks clunky, I'm perfectly happy to keep the "for" loop.


================
Comment at: clang/lib/Lex/ModuleMap.cpp:3059
+    case MMToken::RequiresKeyword:
+      parseRequiresDecl(true);
+      break;
----------------
`/*TopLevel=*/` would help understand the meaning of `true`.

Also I'm not sure if `parseRequiresDecl` parameter should have default value as we can update all the call places. But I don't have a strong opinion about that and leave the decision to you.


================
Comment at: clang/test/Modules/requires-block.m:8
+
+//--- include/module.modulemap
+
----------------
After reading the code it seems more like testing `skipUntil`, so I'm not sure it is a useful test. I was thinking about testing the closing brace as a token, i.e.,

```
requires checkClosingBraceNotAsSeparateToken {
  //}
}
```

If you think it doesn't add extra value, no need to add it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118311/new/

https://reviews.llvm.org/D118311