Sirraide wrote: > Downstream whenever we reach out for a RecursiveASTVisitor we always have to add a few const_casts to shoe it in. This NFC patch introduces a const version of the same CRTP class.
Have you tried using `DynamicRecursiveASTVisitor`/`ConstDynamicRecursiveASTVisitor` instead? It should cover *most* use cases (unless you need to override e.g. `WalkUpFromXY` and friends). I am very much against adding this unless there is a very strong motivation for it that isn’t already covered by `ConstDynamicRecursiveASTVisitor`: Adding even a single instantiation of RAV has a measurable impact on Clang’s compilation speed (even just adding `ConstDynamicRecursiveASTVisitor`, which adds a single instantiation of RAV, came with a compile-time performance regression of about .05% iirc), to the point where we’ve been trying to get rid of uses of RAV in favour of DRAV as much as possible. We don’t exactly want people to start writing *more* RAVs. Even if we never instantiate this new visitor upstream, the base class means there is one extra class to instantiate, and I am worried that even just parsing a huge template like this might be an issue (last time I checked, `RecursiveASTVisitor.h` proper would expand to something like 20 000 LOC because of all the macros...), so if we *really* need this for whatever reason, it should be benchmarked first to make sure it doesn’t introduce a performance regression. https://github.com/llvm/llvm-project/pull/160065 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
