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

[Haojian Wu via Phabricator via cfe-commits]

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