================
@@ -54,6 +54,7 @@
namespace clang {
class ASTContext;
+class DeclStateReverter;
----------------
jaredwy wrote:
> Are you talking about PTUMutationAction or DeclStateReverter?
Both. It's the same problem in each, so start with `DeclStateReverter`.
I think you and Vassil agree, you just don't see it yet.
> It seems to be explicitly check and dispatching on decl kind which seems
> suboptimal to me.
> DeclStateReverter mainly provides a few privileged operations on a
> declaration. In that sense, it is more similar to ASTDeclMerger, which
> provides specific functionality that ASTDeclReader can use.
That's the same position. ASTDeclMerger has no kind dispatch in it, it's typed
primitives (`mergeRedeclarable`, `mergeTemplatePattern`, ...). ASTDeclReader is
the DeclVisitor and it picks the primitive from `VisitFunctionDecl` /
`VisitCXXRecordDecl`. So "like ASTDeclMerger" means pull the `dyn_cast` chains
out of `revertDefinitionArrival`, `tryDetachRedeclChain` and
`removeSpecializations` and let the caller dispatch. Right now those are three
different partitions. One has `FieldDecl`, one keys on `TagDecl` /
`NamespaceDecl`, neither matches DeclShape. That's what I was getting at with
extending ASTDeclUnmerger instead of duplicating it. It also gives you this:
> Ideally, most of the declaration-reverting logic should live directly in
> PTUMutationActions::restoreX.
So the only gap is what the caller is, which is where I started.
> DeclShape is not the completely same as Decl::kind.
Override the six `VisitXDecl` you care about and hierarchy fallthrough gives
you DeclShape, with a typed pointer instead of `cast<TYPE>(D)` off a stored tag.
> we are not performing one single operation for each declaration kind.
> Instead, there are separate operations, and each operation has its own
> dispatch.
That's one visitor per operation. Reader, Writer and Unmerger already work that
way. I still don't see what the switch buys over that.
Once the reverter is Merger shaped the friend question answers itself. Name it
for the primitives it provides, like Merger, not for commit/restore/unlink. The
ledger is the new part and it's orthogonal, `ASTDeclUnmerger` can take it and
keep its dispatch.
https://github.com/llvm/llvm-project/pull/223305
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
