[PATCH] D127448: [wip][pseudo] Implement guard extension.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The ParseLang + CLI library seem clearly separate from everything else, I think this patch should be split.

Comments on that part first...



================
Comment at: clang-tools-extra/pseudo/benchmarks/Benchmark.cpp:68
   for (auto _ : State)
-    LRTable::buildSLR(*G);
+    LRTable::buildSLR(cli::getLanguage().G);
 }
----------------
getLanguage() should not be inside the loop. This is happening in a few places

Maybe it should stay in setupGrammarAndSource?


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Language.h:35
+struct ParseLang {
+  const Grammar &G;
+  const LRTable &Table;
----------------
why are these all references with ParseLang (presumably) passed by value, instead of values with ParseLang passed by reference?


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:24
+struct ParseLang;
+namespace cli {
+// Returns the corresponding language from the '--grammar' command-line flag.
----------------
nit: I don't think a namespace is needed here, compared to just `getLanguageFromFlags()`


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:26
+// Returns the corresponding language from the '--grammar' command-line flag.
+const ParseLang &getLanguage();
+} // namespace cli
----------------
either "language" is a clear enough name or it isn't - given naming elsewhere this should be getParseLang I think


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:298
   auto Pop = [&](const GSS::Node *Head, RuleID RID) {
-    LLVM_DEBUG(llvm::dbgs() << "  Pop " << Params.G.dumpRule(RID) << "\n");
-    const auto &Rule = Params.G.lookupRule(RID);
+    LLVM_DEBUG(llvm::dbgs() << "  Pop " << Params.Lang.G.dumpRule(RID) << "\n");
+    const auto &Rule = Params.Lang.G.lookupRule(RID);
----------------
I'm not sure exactly what the best thing to do about this is, but `Params.Lang.G` is hard to read - it's a long complicated name that doesn't even name the thing.

I think probably we should split ParseParams up instead of nesting ParseLang in it, e.g.

```
struct GLRStorage { ForestArena&, GSS }
glrReduce(vector<ParseStep>, const ParseLang&, GLStorage&, NewHeadCallback)
```

Or even put a forest pointer in the GSS just for convenience.


================
Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:52
+        llvm::errs() << Diag << "\n";
+      std::exit(1);
+    }
----------------
huh? diagnostics aren't fatal (at least haven't been considered so before)


================
Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:53
+      std::exit(1);
+    }
+
----------------
"missing guard function" is probably a grammar diagnostic, maybe we should add some logic to validate extensions?
Probably a FIXME for now


================
Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:57-58
+    llvm::DenseMap<ExtensionID, Guard> *Guards =
+        new llvm::DenseMap<ExtensionID, Guard>();
+    for (ExtensionID ID = 1; ID < G->table().AttributeValues.size(); ++ID)
+      Guards->insert(std::make_pair(ID, alwaysAccept));
----------------
I don't think the CLI library should be in charge of knowing the fallback logic for each type of extension.
Could the parser handle a missing guard itself instead?


================
Comment at: clang-tools-extra/pseudo/tool/ClangPseudo.cpp:94
 
-  if (Grammar.getNumOccurrences()) {
-    std::string Text = readOrDie(Grammar);
-    std::vector<std::string> Diags;
-    auto G = Grammar::parseBNF(Text, Diags);
-
-    if (!Diags.empty()) {
-      llvm::errs() << llvm::join(Diags, "\n");
-      return 2;
-    }
-    llvm::outs() << llvm::formatv("grammar file {0} is parsed successfully\n",
-                                  Grammar);
+  if (true) {
+    const auto &Lang = clang::pseudo::cli::getLanguage();
----------------
drop if(true)?


================
Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:51
   }
-
+  // FIXME: move to TokenStream class.
+  TokenStream emptyTokenStream() {
----------------
either do it or don't - this is too small to check in a fixme IMO


================
Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:57
+  }
+  ParseLang getTestLang() {
+    return {*G, Table, Guards};
----------------
can this just be a member?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127448