hokein added inline comments.
================
Comment at: clang-tools-extra/pseudo/benchmarks/Benchmark.cpp:51
const std::string *SourceText = nullptr;
-const Grammar *G = nullptr;
+const Language *PLang = nullptr;
----------------
sammccall wrote:
> nit: still PLang here and in a bunch of places
renamed all to `Lang`
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:1
+//===--- ParseLang.h ------------------------------------------- -*-
C++-*-===//
+//
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > The "ParseLang" name doesn't feel right to me :-(
> > >
> > > I think it's a combination of:
> > > - Lang is unneccesarily abbreviated
> > > - "Parse" doesn't actually disambiguate much, as "parse" is the whole
> > > project
> > >
> > > Do you think `clang::pseudo::Language` would work?
> > >
> > >
> > > (Sorry for not realizing this on the previous pass, I know it's a pain...
> > > happy to chat more offline)
> > That sounds good to me. Regarding the location of this file, I think the
> > subdir grammar will be a better fit.
> The main purpose of the `grammar` library is to minimize the amount of stuff
> we pull into the gen step right?
>
> I'm a bit concerned about mixing clang::LangOptions in there unneccesarily -
> if grammar doesn't *need* the header, maybe it's OK where it is?
as discussed offline, moved back to `pseudo/`
================
Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:42
+ auto G = Grammar::parseBNF(GrammarText->get()->getBuffer(), Diags);
+ if (!Diags.empty()) {
+ for (const auto &Diag : Diags)
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > this if() isn't needed unless you want to print a header to provide some
> > > context (which might be a good idea)
> > I don't get your point of the comment. Not sure what you meant by `Print
> > a header`?
> >
> > I think for the CLI tool use-case, printing the diagnostic of the grammar
> > by default is reasonable.
> You could replace
>
> ```
> if (!Diags.empty())
> for(D : Diags)
> ...
> ```
>
> with just:
> ```
> for (D : Diags)
> ...
> ```
>
> unless you would prefer:
> ```
> if (!Diags.empty()) {
> errs() << "Problems with the grammar:\n";
> for (D : Diags)
> ...
> }
> ```
>
> (Yesterday I thought the last one would be clearer, today I'm not so sure)
oops, I see what you mean now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128679/new/
https://reviews.llvm.org/D128679
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
