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

[Michael Spencer via Phabricator via llvm-commits]

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 &LangOpts,
+                         const TargetInfo &Target);
----------------
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