[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 for whenever a match fails
`-Rmodule-requires-fail` or something like that. I've been bitten before by a
regular submodule `requires` not triggering, just to find out some specific
subfeature was missing in the compiler invocation - it can take a while to spot
that. Doesn't need to be a blocker for getting this in though.
Comment at: clang/docs/Modules.rst:657
+}
+ }
+
Nice doc and examples!
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:723
+ "invalid requires declaration outside of a module declaration, did you mean"
+ " to add '{' to open a requires block?">;
def err_mmap_expected_rbrace : Error<"expected '}'">;
I'm not really too worried about a fixit here, but it would be nice if the "did
you mean..." part came out of a note instead.
Comment at: clang/lib/Lex/ModuleMap.cpp:2355
+parseModuleMembers();
+ }
+}
Too many curly braces?
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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 : 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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 hard to get the source location of the end
of the line containing the requires feature list.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118311/new/
https://reviews.llvm.org/D118311
Files:
clang/docs/Modules.rst
clang/include/clang/Basic/DiagnosticLexKinds.td
clang/include/clang/Basic/Module.h
clang/lib/Basic/Module.cpp
clang/lib/Lex/ModuleMap.cpp
clang/test/Modules/requires-block-errors.m
clang/test/Modules/requires-block.m
Index: clang/test/Modules/requires-block.m
===
--- /dev/null
+++ clang/test/Modules/requires-block.m
@@ -0,0 +1,73 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.mm
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/B.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/B.mm -verify
+
+//--- include/module.modulemap
+
+requires !cplusplus {
+ module A {
+header "A.h"
+ }
+}
+
+requires cplusplus {
+ module A {
+header "A.h"
+header "A.hpp"
+ }
+}
+
+// Test empty blocks
+requires cplusplus {}
+requires !cplusplus {}
+
+// Test nested requires
+requires objc {
+ module B {
+requires !cplusplus {
+ header "B.h"
+}
+requires cplusplus {
+ header "B.hpp"
+}
+ }
+}
+
+//--- include/A.h
+
+typedef int A_t;
+
+//--- include/A.hpp
+
+using Adrgdrg_t = long;
+
+//--- include/B.h
+
+typedef int B_t;
+
+//--- include/B.hpp
+
+using Bdrgdrg_t = long;
+
+//--- A.m
+@import A;
+A_t a;
+Adrgdrg_t b; // expected-error {{unknown type name 'Adrgdrg_t'}}
+
+//--- A.mm
+@import A;
+A_t a;
+Adrgdrg_t b;
+
+//--- B.m
+@import B;
+B_t a;
+Bdrgdrg_t b; // expected-error {{unknown type name 'Bdrgdrg_t'}}
+
+//--- B.mm
+@import B;
+B_t a; // expected-error {{unknown type name 'B_t'}}
+Bdrgdrg_t b;
Index: clang/test/Modules/requires-block-errors.m
===
--- /dev/null
+++ clang/test/Modules/requires-block-errors.m
@@ -0,0 +1,31 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-closing-brace %t/missing-closing-brace.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-opening-brace %t/missing-opening-brace.m -verify
+
+//--- missing-closing-brace/module.modulemap
+
+requires !cplusplus {
+ module Pony {
+header "Pony.h"
+ }
+
+//--- missing-closing-brace/Pony.h
+
+//--- missing-closing-brace.m
+// expected-error@module.modulemap:7 {{expected '}'}}
+// expected-note@module.modulemap:2 {{to match this '{'}}
+@import Pony // expected-error {{could not build module 'Pony'}}
+
+//--- missing-opening-brace/module.modulemap
+
+requires !cplusplus
+module Pony {
+ header "Pony.h"
+}
+
+//--- missing-opening-brace/Pony.h
+
+//--- missing-opening-brace.m
+// expected-error@module.modulemap:2 {{invalid requires declaration outside of a module declaration, did you mean to add '{' to open a requires block?}}
+@import Pony // expected-error {{could not build module 'Pony'}}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1468,10 +1468,19 @@
using ModuleId = SmallVector, 2>;
+struct RequiresFeature {
+ bool RequiredState;
+ std::string Feature;
+};
+using RequiresFeatureList = SmallVector;
+
bool parseModuleId(ModuleId );
void parseModuleDecl();
+void parseModuleMembers();
void parseExternModuleDecl();
-void parseRequiresDecl();
+void parseRequiresFeatureList(RequiresFeatureList );
+void parseRequiresDecl(bool TopLevel = false);
+void parseRequiresBlockBody(const RequiresFeatureList );
void parseHeaderDecl(MMToken::TokenKind, SourceLocation LeadingLoc);
void parseUmbrellaDirDecl(SourceLocation UmbrellaLoc);
void parseExportDecl();
@@ -1825,6 +1834,7 @@
///
/// module-member:
/// requires-declaration
+/// requires-block-declaration
/// header-declaration
/// submodule-declaration
/// export-declaration
@@ -2030,6 +2040,38 @@
ActiveModule->ModuleMapIsPrivate)
diagnosePrivateModules(ExplicitLoc, FrameworkLoc);
+ parseModuleMembers();
+
+ if
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 an else-requires chain by hand would be
> quite error-prone, and it might be annoying to stage the adoption
> separately...)
I don't really expect users to actually need that, but adding it is pretty
trivial.
Comment at: clang/docs/Modules.rst:473
*module-declaration**
+*requires-block**
dexonsmith wrote:
> Should this be `*requires-block-declaration*`? I'm not seeing the definition
> of `*requires-block*` anywhere.
Yes.
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 '}'">;
dexonsmith wrote:
> Bigcheese wrote:
> > vsapsai wrote:
> > > s/rquires/requires/
> > >
> > > Would it be useful to put `requires` into some quotation marks to show
> > > it's not a part of the sentence but used verbatim?
> > Possibly, but I don't think we do that anywhere else. '' is always used to
> > refer to user identifiers, "" is only used when referring to headers or
> > strings, and I don't see any usage of ``. I have added a note so it shows
> > up now as:
> >
> > ```
> > requires-block-errors.m.tmp/missing-opening-brace/module.modulemap:3:1:
> > error: expected '{' to start requires block
> > module Pony {
> > ^
> > requires-block-errors.m.tmp/missing-opening-brace/module.modulemap:2:1:
> > note: for requires block declared here
> > requires !cplusplus
> > ^
> > ```
> >
> > Which makes it clear.
> The updated text looks better; maybe good enough; but I wonder if it'd be
> more clear to diagnose as a requires-declaration at global scope. E.g.,
> something like:
> ```
> error: invalid requires declaration outside of a module
> note: did you mean to add a '{' to open a block?
> ```
> (maybe my wording isn't great but I hope it indicates the direction I'm
> suggesting)
>
> (I don't feel strongly either way)
That does seem better, although I'm not sure how difficult it is to emit a
fixit from the modulemap parser, but I can take a look.
Comment at: clang/include/clang/Basic/Module.h:249
+ /// language options has the given feature.
+ static bool hasFeature(StringRef Feature, const LangOptions ,
+ const TargetInfo );
iana wrote:
> Is `static` correct? (Sorry I know very little about C++)
Yeah, you don't need an instance of a `Module` to query this.
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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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:
> vsapsai wrote:
> > s/rquires/requires/
> >
> > Would it be useful to put `requires` into some quotation marks to show it's
> > not a part of the sentence but used verbatim?
> Possibly, but I don't think we do that anywhere else. '' is always used to
> refer to user identifiers, "" is only used when referring to headers or
> strings, and I don't see any usage of ``. I have added a note so it shows up
> now as:
>
> ```
> requires-block-errors.m.tmp/missing-opening-brace/module.modulemap:3:1:
> error: expected '{' to start requires block
> module Pony {
> ^
> requires-block-errors.m.tmp/missing-opening-brace/module.modulemap:2:1: note:
> for requires block declared here
> requires !cplusplus
> ^
> ```
>
> Which makes it clear.
The updated text looks better; maybe good enough; but I wonder if it'd be more
clear to diagnose as a requires-declaration at global scope. E.g., something
like:
```
error: invalid requires declaration outside of a module
note: did you mean to add a '{' to open a block?
```
(maybe my wording isn't great but I hope it indicates the direction I'm
suggesting)
(I don't feel strongly either way)
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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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? (Sorry I know very little about C++) 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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 an else-requires chain by hand would be
quite error-prone, and it might be annoying to stage the adoption separately...)
Comment at: clang/docs/Modules.rst:473
*module-declaration**
+*requires-block**
Should this be `*requires-block-declaration*`? I'm not seeing the definition of
`*requires-block*` anywhere.
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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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/
https://reviews.llvm.org/D118311
Files:
clang/docs/Modules.rst
clang/include/clang/Basic/DiagnosticLexKinds.td
clang/include/clang/Basic/Module.h
clang/lib/Basic/Module.cpp
clang/lib/Lex/ModuleMap.cpp
clang/test/Modules/requires-block-errors.m
clang/test/Modules/requires-block.m
Index: clang/test/Modules/requires-block.m
===
--- /dev/null
+++ clang/test/Modules/requires-block.m
@@ -0,0 +1,73 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.mm
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/B.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/B.mm -verify
+
+//--- include/module.modulemap
+
+requires !cplusplus {
+ module A {
+header "A.h"
+ }
+}
+
+requires cplusplus {
+ module A {
+header "A.h"
+header "A.hpp"
+ }
+}
+
+// Test empty blocks
+requires cplusplus {}
+requires !cplusplus {}
+
+// Test nested requires
+requires objc {
+ module B {
+requires !cplusplus {
+ header "B.h"
+}
+requires cplusplus {
+ header "B.hpp"
+}
+ }
+}
+
+//--- include/A.h
+
+typedef int A_t;
+
+//--- include/A.hpp
+
+using Adrgdrg_t = long;
+
+//--- include/B.h
+
+typedef int B_t;
+
+//--- include/B.hpp
+
+using Bdrgdrg_t = long;
+
+//--- A.m
+@import A;
+A_t a;
+Adrgdrg_t b; // expected-error {{unknown type name 'Adrgdrg_t'}}
+
+//--- A.mm
+@import A;
+A_t a;
+Adrgdrg_t b;
+
+//--- B.m
+@import B;
+B_t a;
+Bdrgdrg_t b; // expected-error {{unknown type name 'Bdrgdrg_t'}}
+
+//--- B.mm
+@import B;
+B_t a; // expected-error {{unknown type name 'B_t'}}
+Bdrgdrg_t b;
Index: clang/test/Modules/requires-block-errors.m
===
--- /dev/null
+++ clang/test/Modules/requires-block-errors.m
@@ -0,0 +1,32 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-closing-brace %t/missing-closing-brace.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-opening-brace %t/missing-opening-brace.m -verify
+
+//--- missing-closing-brace/module.modulemap
+
+requires !cplusplus {
+ module Pony {
+header "Pony.h"
+ }
+
+//--- missing-closing-brace/Pony.h
+
+//--- missing-closing-brace.m
+// expected-error@module.modulemap:7 {{expected '}'}}
+// expected-note@module.modulemap:2 {{to match this '{'}}
+@import Pony // expected-error {{could not build module 'Pony'}}
+
+//--- missing-opening-brace/module.modulemap
+
+requires !cplusplus
+module Pony {
+ header "Pony.h"
+}
+
+//--- missing-opening-brace/Pony.h
+
+//--- missing-opening-brace.m
+// expected-error@module.modulemap:3 {{expected '{' to start requires block}}
+// expected-note@module.modulemap:2 {{for requires block declared here}}
+@import Pony // expected-error {{could not build module 'Pony'}}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1468,10 +1468,19 @@
using ModuleId = SmallVector, 2>;
+struct RequiresFeature {
+ bool RequiredState;
+ std::string Feature;
+};
+using RequiresFeatureList = SmallVector;
+
bool parseModuleId(ModuleId );
void parseModuleDecl();
+void parseModuleMembers();
void parseExternModuleDecl();
-void parseRequiresDecl();
+void parseRequiresFeatureList(RequiresFeatureList );
+void parseRequiresDecl(bool TopLevel = false);
+void parseRequiresBlockBody(const RequiresFeatureList );
void parseHeaderDecl(MMToken::TokenKind, SourceLocation LeadingLoc);
void parseUmbrellaDirDecl(SourceLocation UmbrellaLoc);
void parseExportDecl();
@@ -1825,6 +1834,7 @@
///
/// module-member:
/// requires-declaration
+/// requires-block-declaration
/// header-declaration
/// submodule-declaration
/// export-declaration
@@ -2030,6 +2040,38 @@
ActiveModule->ModuleMapIsPrivate)
diagnosePrivateModules(ExplicitLoc, FrameworkLoc);
+ parseModuleMembers();
+
+ if (Tok.is(MMToken::RBrace))
+consumeToken();
+ else {
+Diags.Report(Tok.getLocation(), diag::err_mmap_expected_rbrace);
+Diags.Report(LBraceLoc, diag::note_mmap_lbrace_match);
+HadError = true;
+ }
+
+ // If the active
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 have some mechanism to notify developers that includes
> are still performed regardless of requires.
This would be nice, but is rather complicated to do given how the module map
parser works right now. We would need to actually record what gets skipped by a
requires block without including it in the `Module`. With the current design
that's not easy to do.
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 '}'">;
vsapsai wrote:
> s/rquires/requires/
>
> Would it be useful to put `requires` into some quotation marks to show it's
> not a part of the sentence but used verbatim?
Possibly, but I don't think we do that anywhere else. '' is always used to
refer to user identifiers, "" is only used when referring to headers or
strings, and I don't see any usage of ``. I have added a note so it shows up
now as:
```
requires-block-errors.m.tmp/missing-opening-brace/module.modulemap:3:1: error:
expected '{' to start requires block
module Pony {
^
requires-block-errors.m.tmp/missing-opening-brace/module.modulemap:2:1: note:
for requires block declared here
requires !cplusplus
^
```
Which makes it clear.
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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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` block? I mean
that in one context the module has some extra requirements but in a different
context doesn't have them.
It would be nice to have some mechanism to notify developers that includes are
still performed regardless of `requires`. For example, in A.h we have
#include
and in module map
module A {
header "A.h"
requires cplusplus {
header "A_cpp_feature.h"
}
export *
}
It doesn't mean the header A_cpp_feature.h is used only with cplusplus feature
which is not obvious and with big headers can be hard to notice. But I feel
like this request goes beyond the scope of your change, so not insisting on it.
Also it can be nice to diagnose strange conditions like `requires cplusplus,
!cplusplus` but that's orthogonal to this feature.
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 '}'">;
s/rquires/requires/
Would it be useful to put `requires` into some quotation marks to show it's not
a part of the sentence but used verbatim?
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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 this valid?
> >
> > ```
> > requires cplusplus11 {
> > ...
> > }
> > requires cplusplus && !cplusplus11 {
> > ...
> > }
> > ```
> > Or would you nest the `requires`? If you wanted OR, would you just have to
> > duplicate the requires block body?
> There's currently no `else` syntax, although I don't think it would be hard
> to add. Nesting is allowed. The syntax in your example would be:
>
> ```
> requires cplusplus11 {
> ...
> }
> requires cplusplus, !cplusplus11 {
> ...
> }
> ```
>
> OR currently requires duplication. The idea is that the feature list
> expressions just aren't that complex in practice.
Seems like you don't strictly need `else` then, probably fine without it as
long as there are enough examples in the documentation?
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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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?
>
> ```
> requires cplusplus11 {
> ...
> }
> requires cplusplus && !cplusplus11 {
> ...
> }
> ```
> Or would you nest the `requires`? If you wanted OR, would you just have to
> duplicate the requires block body?
There's currently no `else` syntax, although I don't think it would be hard to
add. Nesting is allowed. The syntax in your example would be:
```
requires cplusplus11 {
...
}
requires cplusplus, !cplusplus11 {
...
}
```
OR currently requires duplication. The idea is that the feature list
expressions just aren't that complex in practice.
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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 {
...
}
requires cplusplus && !cplusplus11 {
...
}
```
Or would you nest the `requires`? If you wanted OR, would you just have to
duplicate the requires block body?
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
[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration
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 declaration.
A requires block declaration allows conditional inclusion of other
declarations within a module map.
The syntax is the same as a requires-declaration, except that it is
followed by a block. If the feature-list isn’t satisfied, then the
contents of the block is ignored and skipped over. If the feature-list
is satisfied, then the contents of the requires block are parsed as if
the requires block was not present.
This differs from a requires-declaration in that it is not an error to
import a module from within an unsatisfied requires-block-declaration
as long as another declaration of the module exists.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D118311
Files:
clang/docs/Modules.rst
clang/include/clang/Basic/DiagnosticLexKinds.td
clang/include/clang/Basic/Module.h
clang/lib/Basic/Module.cpp
clang/lib/Lex/ModuleMap.cpp
clang/test/Modules/requires-block-errors.m
clang/test/Modules/requires-block.m
Index: clang/test/Modules/requires-block.m
===
--- /dev/null
+++ clang/test/Modules/requires-block.m
@@ -0,0 +1,64 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.mm
+
+//--- include/module.modulemap
+
+requires !cplusplus {
+ module A {
+header "A.h"
+ }
+}
+
+requires cplusplus {
+ module A {
+header "A.h"
+header "A.hpp"
+ }
+}
+
+module B {
+ requires !cplusplus {
+header "B.h"
+ }
+ requires cplusplus {
+header "B.hpp"
+ }
+}
+
+//--- include/A.h
+
+typedef int A_t;
+
+//--- include/A.hpp
+
+using Adrgdrg_t = long;
+
+//--- include/B.h
+
+typedef int B_t;
+
+//--- include/B.hpp
+
+using Bdrgdrg_t = long;
+
+//--- A.m
+@import A;
+A_t a;
+Adrgdrg_t b; // expected-error {{unknown type name 'Adrgdrg_t'}}
+
+//--- A.mm
+@import A;
+A_t a;
+Adrgdrg_t b;
+
+//--- B.m
+@import B;
+B_t a;
+Bdrgdrg_t b; // expected-error {{unknown type name 'Bdrgdrg_t'}}
+
+//--- B.mm
+@import B;
+B_t a; // expected-error {{unknown type name 'B_t'}}
+Bdrgdrg_t b;
Index: clang/test/Modules/requires-block-errors.m
===
--- /dev/null
+++ clang/test/Modules/requires-block-errors.m
@@ -0,0 +1,31 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-closing-brace %t/missing-closing-brace.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-opening-brace %t/missing-opening-brace.m -verify
+
+//--- missing-closing-brace/module.modulemap
+
+requires !cplusplus {
+ module Pony {
+header "Pony.h"
+ }
+
+//--- missing-closing-brace/Pony.h
+
+//--- missing-closing-brace.m
+// expected-error@module.modulemap:7 {{expected '}'}}
+// expected-note@module.modulemap:2 {{to match this '{'}}
+@import Pony // expected-error {{could not build module 'Pony'}}
+
+//--- missing-opening-brace/module.modulemap
+
+requires !cplusplus
+module Pony {
+ header "Pony.h"
+}
+
+//--- missing-opening-brace/Pony.h
+
+//--- missing-opening-brace.m
+// expected-error@module.modulemap:3 {{expected '{' to start rquires block}}
+@import Pony // expected-error {{could not build module 'Pony'}}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1468,10 +1468,19 @@
using ModuleId = SmallVector, 2>;
+struct RequiresFeature {
+ bool RequiredState;
+ std::string Feature;
+};
+using RequiresFeatureList = SmallVector;
+
bool parseModuleId(ModuleId );
void parseModuleDecl();
+void parseModuleMembers();
void parseExternModuleDecl();
-void parseRequiresDecl();
+void parseRequiresFeatureList(RequiresFeatureList );
+void parseRequiresDecl(bool TopLevel = false);
+void parseRequiresBlockBody(const RequiresFeatureList );
void parseHeaderDecl(MMToken::TokenKind, SourceLocation LeadingLoc);
void parseUmbrellaDirDecl(SourceLocation UmbrellaLoc);
void parseExportDecl();
@@ -1825,6 +1834,7 @@
///
/// module-member:
/// requires-declaration
+/// requires-block-declaration
/// header-declaration
/// submodule-declaration
/// export-declaration
@@ -2030,6 +2040,38 @@
ActiveModule->ModuleMapIsPrivate)
diagnosePrivateModules(ExplicitLoc, FrameworkLoc);
+ parseModuleMembers();
+
+
