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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits