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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, the split here looks good!



================
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 {
----------------
You're already touching the callsites of the glr* functions, it might be worth just doing this already... up to you


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


================
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 {
----------------
I don't know what this sentence means - is it always true? is it necessary?


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:24
+
+  // FIXME: add clang::LangOptions.
+  // FIXME: add default start symbols.
----------------
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)


================
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
----------------
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?


================
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();
+
----------------
this should generally be called exactly one from CLI tools - why does it need to be memoized and returned as a reference?


================
Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:38
+                   << "': " << EC.message() << "\n";
+      std::exit(1);
+    }
----------------
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


================
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)
----------------
this if() isn't needed unless you want to print a header to provide some context (which might be a good idea)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128679