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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:115
 // Parameters for the GLR parsing.
+// FIXME: refine it with the ParseLang struct.
 struct ParseParams {
----------------
sammccall wrote:
> You're already touching the callsites of the glr* functions, it might be worth just doing this already... up to you
these callsites are trivial to update. I'd prefer to do it in a separate patch.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:1
+//===--- ParseLang.h ------------------------------------------- -*- C++-*-===//
+//
----------------
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.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:19
+// Specify a language that can be parsed by the pseduoparser.
+// Manifest generated from a bnf grammar file.
+struct ParseLang {
----------------
sammccall wrote:
> I don't know what this sentence means - is it always true? is it necessary?
I think it is always true for the grammar and LRTable object, but might not be true for the LangOptions. Removed.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:24
+
+  // FIXME: add clang::LangOptions.
+  // FIXME: add default start symbols.
----------------
sammccall wrote:
> is this "fix right after this patch" or "fix sometime, maybe" or something in between?
> 
> (I think these make a lot of sense, and am worried the structure will be incoherent if we don't actually add them soon)
I would expect we will add them soon.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:9
+//
+// A library shared among different pseudoparser-based tools. It provides a
+// uniform way to get basic pieces of the parser (Grammar, LRTable etc) from
----------------
sammccall wrote:
> The first sentence should say what it is, rather than what calls it.
> 
> If the file by design defines a single thing function I'm not sure a file comment distinct from the function comment helps much, maybe merge them?
I'm not sure whether we will add more functions in this library (though I can't come up with one), I will just keep the file comment.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:27
+// Returns the corresponding language from the '--grammar' command-line flag.
+const ParseLang &getParseLangFromFlags();
+
----------------
sammccall wrote:
> this should generally be called exactly one from CLI tools - why does it need to be memoized and returned as a reference?
yeah, for now, returning an object is ok, but in the future, when we build the ParseLang (LRTable, LRGrammar) for cxx.bnf in a fully-static way, we might have trouble right?


================
Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:38
+                   << "': " << EC.message() << "\n";
+      std::exit(1);
+    }
----------------
sammccall wrote:
> this is extremely surprising.
> At minimum it needs to be very clearly documented, but I think it's probably better to return null to the caller
It doesn't seem to be surprising to me, as this function is used for the CLI tools, crashing the program when the input grammar text is invalid seems reasonable (all our exiting CLI tools at the moment behave like this), and we have print the error message. 

Returning a nullptr seems annoying IMO -- it requires all callers to handle this unimportant case. Updated the document for this function.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128679/new/

https://reviews.llvm.org/D128679