[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

labath added a comment. In D60410#1464214 , @teemperor wrote: > I believe this revision introduced a warning when compiling with Clang: > >98% [4004/4047] Building CXX object > tooldir/PdbFPOProgramToDWARFExpression.cpp.o > > /home/teemperor/ll

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

teemperor added a comment. I believe this revision introduced a warning when compiling with Clang: 98% [4004/4047] Building CXX object tooldir/PdbFPOProgramToDWARFExpression.cpp.o /home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpres

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB358261: PDBFPO: Improvements to the AST visitor (authored by labath, committed by ). Herald added a subscriber: abidh. Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

amccarth accepted this revision. amccarth added a comment. I'm even happier. Thanks for giving the params more specific names. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60410/new/ https://reviews.llvm.org/D60410 ___ lldb-commits mailin

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

labath updated this revision to Diff 194705. labath added a comment. After trying to use this in new code, I realized that the CRTP is not really needed for what I am trying to achieve here. The same can be achieved through more standard virtual functions. This updates the patch to use virtual fu

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Overall, I'm ambivalent. The patch description makes a good case for the change. I find the visitor pattern hard to follow (partly because the `visit` and `accept` naming is not intuitiv

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

labath updated this revision to Diff 194160. labath added a comment. Fix a bug which meant we weren't replacing the root node correctly (I somehow forgot to run tests before uploading). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60410/new/ https://reviews.llvm.org/D60410 Files: so

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

labath added a comment. I find this version easier to follow than the old one, but that could be simply because I didn't write it. :P So if you believe the old one is better, I am happy to just drop this and carry on with the old one. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60410

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

labath created this revision. labath added reviewers: aleksandr.urakov, amccarth. This patch attempts to solve two issues made this code hard to follow for me. The first issue was that a lot of what these visitors do is mutate the AST. The visitor pattern is not particularly good for that because