sammccall marked 2 inline comments as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:208
+ // Underlying storage for sequences pointed to by stored SequenceRefs.
+ std::deque<Sequence> SequenceStorage;
+ // We don't actually destroy the sequences between calls, to reuse storage.
----------------
hokein wrote:
> I start to have a nervous feeling that we're going to add more and more
> intermediate data structure, which increases the complexity of the code, but
> I think the improvement is large enough to justify.
Agree :-(
================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:270
+
+ Sequences.emplace(F, PushSpec{N, Seq});
return;
----------------
hokein wrote:
> Just an idea (no action required)
>
> If we want to do further optmization, here if a sequence have multiple Bases,
> we will have multiple elements in `Sequences` -- they have the form of (F,
> PushSpec{N, Seq}) where only N (the base node) is different.
>
> - if we change the PushSpec::Base to store the child node of Base, we could
> save a bunch of space in Sequences. (The base can be retrieved dynamically
> from the `child->parents()`).
This is interesting! Essentially the reason we do the DFS eagerly (and pay to
store its results) is we have to establish the Family via the start token, but
this means we have to go only as far as the end of the sequence, rather than
one further. (One further feels "cleaner" somehow, but who cares...)
I'll give this a try (not in this patch)
================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:317
FamilySequences.emplace_back(Sequences.top().first.Rule,
- Sequences.top().second.Seq);
+ *Sequences.top().second.Seq); // XXX
FamilyBases.emplace_back(
----------------
hokein wrote:
> What's XXX?
Oops, this was left-over from when Sequences stored by pointer and
FamilySequences stored by value. Removed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128307/new/
https://reviews.llvm.org/D128307
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
