hokein added a comment. Nice! I really like the form it goes now.
================ Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:82 // FIXME: Most nodes live a fairly short time, and are simply discarded. // Is it worth refcounting them (we have empty padding) and returning to a ---------------- This FIXME is done by this patch :) ================ Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:15 #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/StringExtras.h" ---------------- an off-topic comment: we only use the function in debug branch code, this will trigger the include-cleaner warning in release mode :) we could warp it with `#ifndef NDEBUG` as well. ================ Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:87 NewHeads.clear(); + MaybeGC(); glrReduce(PendingReduce, Params, ---------------- I would just call it before the `NewHeads.clear()`, and run the `gc` on the `NewHeads` directly. It simplifies the implementation (no need to traverse three pending actions, and no duplicated elements in Roots). The downside is that some dead heads (heads with no available actions on `Terminal.symbol()`) will not be GCed in this run, but I think it is negligible, as those will be GCed in next run. ================ Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:391 + llvm::ArrayRef<const Node *> Parents) { + ++NodeCount; + Node *Result = new (allocate(Parents.size())) ---------------- The NodeCount becomes vague now, we update in both addNode and allocate methods, I think we should have two separate counts. ================ Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:403 + ++NodeCount; + if (FreeList.size() <= Parents) + FreeList.resize(Parents + 1); ---------------- What's the practical size of the `FreeList`? I think we can just find one, and use it as initial default size, to save some cost of resize. ================ Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:412 + EXPECT_EQ(0u, GSStack.gc({AB, C})); // D is already gone. + auto *E = GSStack.addNode(0, nullptr, {Root}); + EXPECT_EQ(3u, GSStack.gc({A, E})); // destroys B, AB, C ---------------- nit: since we free D, I think here E will reuse the memory free from D, maybe add pointer comparison with `E` and `D` ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126723/new/ https://reviews.llvm.org/D126723 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits