[PATCH] D128679: [pseudo] Define a clangPseudoCLI library.

[Sam McCall via Phabricator via cfe-commits]

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