sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/pseudo/benchmarks/Benchmark.cpp:51
const std::string *SourceText = nullptr;
-const Grammar *G = nullptr;
+const Language *PLang = nullptr;
----------------
nit: still PLang here and in a bunch of places
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:1
+//===--- ParseLang.h ------------------------------------------- -*-
C++-*-===//
+//
----------------
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?
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:9
+//
+// This file implements a CLI library, which provides an interface for getting
+// the basic pieces of psuedoparser (Grammar, LRTable etc) from a variety of
----------------
nit: "this file implements a CLI library which provides an interface for" is
boilerplate.
Just "Provides the Grammar, LRTable etc for a language specified by flags"?
================
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)
----------------
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)
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
