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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jun 27 15:00:51 PDT 2022

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)

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list