Bigcheese added inline comments.
Comment at: clang/lib/Lex/ModuleMap.cpp:2355
+parseModuleMembers();
+ }
+}
bruno wrote:
> Too many curly braces?
This is correct. It closes the block on line 2353.
Repository:
rG LLVM Github Monorepo
CHANGES
bruno requested changes to this revision.
bruno added a comment.
This revision now requires changes to proceed.
Herald added a project: All.
> It would be nice to have some mechanism to notify developers that includes
> are still performed regardless of requires
I'd like to see a module remark
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"
}
}
//
Bigcheese updated this revision to Diff 405482.
Bigcheese added a comment.
- Fixed documentation typo.
- Made missing '{' diagnostic more clear.
- Use `ModuleMapParser::skipUntil`.
Emitting an actual fixup is kind of difficult from `ModuleMapParser` because
the way it handles tokens makes it
Bigcheese added a comment.
> Drive-by comment on the docs; otherwise this sounds awesome; as long as else
> is easy to add later this SGTM (I'll let others do the code review).
> (Although, if else {} and else requires {} would be easy to add now/soon, I
> feel like it'd be worth it. Modelling
dexonsmith added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:722
+def err_mmap_expected_lbrace_requires : Error<
+ "expected '{' to start rquires block">;
def err_mmap_expected_rbrace : Error<"expected '}'">;
Bigcheese wrote:
>
iana added inline comments.
Comment at: clang/include/clang/Basic/Module.h:249
+ /// language options has the given feature.
+ static bool hasFeature(StringRef Feature, const LangOptions ,
+ const TargetInfo );
Is `static` correct?
dexonsmith added a comment.
Drive-by comment on the docs; otherwise this sounds awesome; as long as `else`
is easy to add later this SGTM (I'll let others do the code review).
(Although, if `else {}` and `else requires {}` would be easy to add now/soon, I
feel like it'd be worth it. Modelling
Bigcheese updated this revision to Diff 405133.
Bigcheese added a comment.
Add testing of empty blocks and nested blocks and make the missing `{` error
clearer.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118311/new/
Bigcheese added a comment.
> Is it possible to reference external module map from requires block? I mean
> that in one context the module has some extra requirements but in a different
> context doesn't have them.
Can you provide an example where this would cause issues?
> It would be nice to
vsapsai added a comment.
Haven't really checked the code, at the moment thinking about various failure
modes.
Cases that aren't tested but I suspect are valid ones:
- empty block, i.e., `requires cplusplus {}`
- nested blocks.
Is it possible to reference external module map from `requires`
iana added inline comments.
Comment at: clang/docs/Modules.rst:651
+
+requires cplusplus {
+ header "vector"
Bigcheese wrote:
> iana wrote:
> > Is there any kind of `else` syntax here? Or do you just use `!whatever` for
> > the else? Is something like
Bigcheese added inline comments.
Comment at: clang/docs/Modules.rst:651
+
+requires cplusplus {
+ header "vector"
iana wrote:
> Is there any kind of `else` syntax here? Or do you just use `!whatever` for
> the else? Is something like this valid?
>
>
iana added inline comments.
Comment at: clang/docs/Modules.rst:651
+
+requires cplusplus {
+ header "vector"
Is there any kind of `else` syntax here? Or do you just use `!whatever` for the
else? Is something like this valid?
```
requires cplusplus11 {
Bigcheese created this revision.
Bigcheese added reviewers: bruno, dexonsmith, vsapsai.
Bigcheese added a project: clang-modules.
Bigcheese requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Add a new form of requires called a requires block
15 matches
Mail list logo
