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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Great!



================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:117
 struct ParseParams {
-  // The grammar of the language we're going to parse.
-  const Grammar &G;
-  // The LR table which GLR uses to parse the input, should correspond to the
-  // Grammar G.
-  const LRTable &Table;
+  // The token stream being to parse.
+  const TokenStream &Code;
----------------
delete "being".
Or possibly the whole comment, this seems obvious?


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:254
 private:
+  bool canReduce(ExtensionID GuardID, SequenceRef RHS) const {
+    if (!GuardID)
----------------
nit: hadn't really intended SequenceRef to be a general vocabulary type, you might just want ArrayRef to match the guard signature, up to you


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:258
+    auto It = Lang.Guards.find(GuardID);
+    if (It == Lang.Guards.end()) {
+      llvm::dbgs() << " missing {0} guard implementation for rule {0}\n";
----------------
this should be guarded by LLVM_DEBUG

this whole thing might be clearer with lookup

```
if (auto Guard = Lang.Guards.lookup(GuardID))
  return Guard(RHS.S, Params.Code);

LLVM_DEBUG(...);
return true;
```


================
Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:486
+  )bnf");
+  TestLang.Guards.insert(std::make_pair(
+      extensionID("TestOnly"),
----------------
insert(make_pair(...)) => emplace(...)?


================
Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:489
+      [&](llvm::ArrayRef<const ForestNode *> RHS, const TokenStream &Tokens) {
+        llvm::errs() << "callback!\n";
+        assert(RHS.size() == 1 &&
----------------
remove


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127448