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