[PATCH] D128485: [pseudo] Store shift and goto actions in a compact structure with faster lookup.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/grammar/LRTable.h:72
   // action types, this class becomes less useful. Remove it.
   class Action {
   public:
----------------
The Action can be removed now (I guess your plan is to remove it in a follow-up patch).


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/grammar/LRTable.h:128
   // Expected to be called by LR parsers.
   // REQUIRES: Nonterminal is valid here.
+  llvm::Optional<StateID> getGoToState(StateID State,
----------------
If we're going to change the return type to `llvm::Optional`.

I think we can remove this comment , as the the invariant is not guaranteed by the API now, but rather than moving to the caller.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/grammar/LRTable.h:131
+                                       SymbolID Nonterminal) const {
+    return Gotos.get(numStates() * Nonterminal + State);
+  }
----------------
add nonterminal assertion?


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/grammar/LRTable.h:213
+  // Lookup is constant time with a low factor (no hashing).
+  class SparseTable {
+    using Word = uint64_t;
----------------
As discussed offline, we could be more specific for the name (something like `StateIDTable`).

We might also want to put the key calculation logic (`numStates() * Nonterminal + State`, and `numStates() * symbolToToken(Terminal) + State`) to an inline function, we have a few places using them duplicately. 


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/grammar/LRTable.h:223
+    SparseTable() = default;
+    SparseTable(const llvm::DenseMap<unsigned, StateID> &V, unsigned NumKeys) {
+      assert(
----------------
nit: V => KV
it feels more natural to make it a static `build` method, but up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128485