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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
