This is an archive of the discontinued LLVM Phabricator instance.

PDBFPO: Improvements to the AST visitor
ClosedPublic

Authored by labath on Apr 8 2019, 8:48 AM.

Details

Summary

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 by
the time you have performed the dynamic type dispatch, it's too late to
go back to the parent node, and change its pointer. The previous code
dealt with that relatively elegantly, but it still meant that one had to
perform manual type checks, which is what the visitor pattern is
supposed to avoid.

The second issue was not being able to return values from the Visit
functions, which meant that one had to store function results in member
variables (a common problem with visitor patterns).

Here, I try to solve both problems by making the visitor a CRTP
template. This allows one to parameterize the visitor based on the
return type and pass function results as function results. The mutation
is fascilitated by having each Visit function take two arguments -- a
reference to the object itself (with the correct dynamic type), and a
reference to the parent's pointer to this object.

Although this wasn't my explicit goal here, the fact that we're not
using virtual dispatch anymore (not possible due to return type
templatization) allows us to make the AST nodes trivially destructible,
which is a good thing, since we were not destroying them anyway.

Event Timeline

labath created this revision.Apr 8 2019, 8:48 AM
labath added a comment.Apr 8 2019, 8:51 AM

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.

labath updated this revision to Diff 194160.Apr 8 2019, 9:03 AM

Fix a bug which meant we weren't replacing the root node correctly (I somehow
forgot to run tests before uploading).

amccarth accepted this revision.Apr 8 2019, 4:24 PM

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 intuitive for me). I find CRTP more understandable. And you make a good argument about the visitor pattern not being right for a mutator. So I'm inclined to like this change

But in practice, I don't find the change any easier to read, and the switch on the type _seems_ like a code smell even though it's justified here.

I'm OK with it if nobody else objects.

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
284–285

I actually preferred symbol for this parameter name. node is very generic. At this point, we know the node is a symbol, so using the more specific name would be more consistent.

This revision is now accepted and ready to land.Apr 8 2019, 4:24 PM
labath updated this revision to Diff 194705.Apr 11 2019, 9:03 AM

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 functions, which may be 0.01% slower, but
that certainly does not matter here. I also rename the function arguments from
"node" to more descriptive names.

amccarth accepted this revision.Apr 11 2019, 1:16 PM

I'm even happier. Thanks for giving the params more specific names.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 12:17 AM
Herald added a subscriber: abidh. · View Herald Transcript

I believe this revision introduced a warning when compiling with Clang:

 98% [4004/4047] Building CXX object tool....dir/PdbFPOProgramToDWARFExpression.cpp.o
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<void>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:51: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<void>' requested here
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
                                                  ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:7: warning: '(anonymous namespace)::FPOProgramASTVisitorMergeDependent' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<bool>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:232:14: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<bool>' requested here
    : public FPOProgramASTVisitor<bool> {
             ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:231:7: warning: '(anonymous namespace)::FPOProgramASTVisitorResolveRegisterRefs' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorResolveRegisterRefs
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:310:7: warning: '(anonymous namespace)::FPOProgramASTVisitorDWARFCodegen' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorDWARFCodegen : public FPOProgramASTVisitor<> {
      ^
5 warnings generated.

I believe this revision introduced a warning when compiling with Clang:

 98% [4004/4047] Building CXX object tool....dir/PdbFPOProgramToDWARFExpression.cpp.o
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<void>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:51: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<void>' requested here
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
                                                  ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:7: warning: '(anonymous namespace)::FPOProgramASTVisitorMergeDependent' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<bool>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:232:14: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<bool>' requested here
    : public FPOProgramASTVisitor<bool> {
             ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:231:7: warning: '(anonymous namespace)::FPOProgramASTVisitorResolveRegisterRefs' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorResolveRegisterRefs
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:310:7: warning: '(anonymous namespace)::FPOProgramASTVisitorDWARFCodegen' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorDWARFCodegen : public FPOProgramASTVisitor<> {
      ^
5 warnings generated.

Thanks for the heads up. I'll fix that right away.