[PSA] Deprecating {Attribute,Type,Value}::{cast,isa,isa_and_nonnull,dyn_cast,dyn_cast_or_null} methods
Context
As noted on the deprecation page, Deprecations & Current Refactoring - MLIR, and originally discussed on Discourse, Preferred casting style going forward, free function isa/cast/… functionality is preferred to method calls. Concretely:
// Preferred
llvm::cast<Value>(*this);
// Deprecated
this->cast<Value>();
At this point, Attribute
, Type
, Value
, Op
, Location
, and PointerUnion
classes and subclasses supported both function and method based casts. I have created a clang-tidy check to provide updating of most call sites of these methods to be function calls, and with some manual changes MLIR can mark these methods as deprecated.
One can see what these changes look like in the commits listed here: Commits · llvm/llvm-project · GitHub
Request
Please comment if there are objections to this change, objections to how this is being handled, or requests for how to approach it. Otherwise, I hope to mark the methods in MLIR as deprecated next week and to remove the methods 1-2 months later.
As an example of a potentially valid concern, this change will result in mixed call styles because now some classes are updated to use function calls, while others only support method calls, so please comment if you consider this a blocking concern.
Below, one will find the tooling to automate a large portion of the changes, but I did the .td
file changes manually with find-next + vim-macros
What to Do
The linked branch has 3 commits:
- 2 commits that provide clang-tidy tooling to handle almost all C++ code. They probably could have been combined, but I didn’t verify for sure, so I left them separately. Running the first commit and second commit one after the other covered all C++ code in MLIR. Running at only the second commit handled all of C++ code in Flang.
- 1 commit showing the deprecated methods. Due to
llvm::TypeSwitch
having a call to the dyn_cast method, I did not mark those methods as deprecated.
Note: These commits only handle C++ code that can be analyzed, so C++ code inside .td
files and in macros are not automatically updated.
Note: This clang-tidy does some additional changes that are not strictly necessary:
-
llvm::PointerUnion
calls were updated, although this functionality cannot be easily removed after MLIR call sites are updated. -
dyn_cast_or_null
is replaced withdyn_cast_if_present
Warning: I wanted to begin this announcement, but, assuming no objections, I will update my branch one more time next week at the same time that I announce the deprecation and then update the commands here with the correct hashes.
As an example, updating Flang could be done with:
curl https://github.com/tpopp/llvm-project/compare/fe7afcf70c93223a16ec7a2a5e07c4ace16c9a04...tpopp:llvm-project:f5d2f889fe00d3396494ad8e605bbee47e046d20.patch | git apply
git add -u
git commit -m "temporary clang-tidy patch"
ninja -C $BUILD_DIR check-flang
ninja -C $BUILD_DIR clang-tidy
run-clang-tidy -clang-tidy-binary=$BUILD_DIR/bin/clang-tidy -checks='-*,misc-cast-functions' flang/* -header-filter=flang/ -fix
One will afterwards have to manually update any other code that is missed, such as the mentioned macros and .td
files.
What is Incomplete
I did not have time to carry this farther to completion. The process of deprecating Location’s methods is incomplete but should be short and easy when someone handles it. Furthermore, any classes that did not already provide the needed functionality for llvm::cast()
style calls are not updated now. An example of this is AffineExpr
which only provides the methods.
Bonus Request
If one is an owner of a class, such as AffineExpr
, please consider implementing LLVM’s CastInfo
for your class, so the usage of cast/isa becomes more consistent. Almost all call sites of your class’ methods should be updated simply by patching my clang-tidy script and adding a call AddMatchers("::mlir::YourClass", "Callee");
before rerunning the clang-tidy
process
Thank you
All feedback is welcome
Tres