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
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.
================
Comment at:
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:284
-bool FPOProgramASTVisitorResolveRegisterRefs::TryReplace(
- FPOProgramNode *&node_ref) {
- auto *symbol = llvm::dyn_cast<FPOProgramNodeSymbol>(node_ref);
- if (!symbol)
- return true;
-
+bool FPOProgramASTVisitorResolveRegisterRefs::Visit(FPOProgramNodeSymbol &node,
+ FPOProgramNode *&ref) {
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60410/new/
https://reviews.llvm.org/D60410
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
