Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIR][NFC] Replace uses of isa/dyn_cast/cast/... member functions #703

Merged
merged 6 commits into from
Jun 26, 2024

Conversation

jopperm
Copy link
Contributor

@jopperm jopperm commented Jun 26, 2024

Mechanical rewrite to use the corresponding free functions; fixes #702.

I used a slightly modified version of the clang-tidy check provided in https://discourse.llvm.org/t/psa-deprecating-cast-isa-methods-in-some-classes/70909 to rewrite the C++ source files, regular expressions for the TableGen files, and manual cleanups where needed (e.g. chains like x.foo().cast<A>().bar().cast<B>())

I applied the following heuristic to determine which namespace prefix to use:

  • If the target type is not qualified, and the TU has using namespace mlir or the code is inside the mlir namespace -> use a plain isa/cast/...
    • Exception: Always qualify inside custom types and attributes, because their base classes define the very members we want to get rid of.
  • Else. i.e. the target type is qualified as ::mlir:: or mlir::, use that prefix.

The clang-tidy check also rewrote dyn_cast_or_null to dyn_cast_if_present. I think that's useful because the former variant is going to be deprecated as well in the future.

I'm using -Werror=deprecated-declarations to test the change (see 6b7420a); this required also changing two occurrences of StringRef::equals to ==. I could also just drop the commit here; maybe we want to enable -Werror in general (there aren't too many other warnings left in the codebase).

Signed-off-by: Julian Oppermann <[email protected]>
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks a bunch! LGTM

if (MSVC)
add_compile_options("/we4996")
else()
add_compile_options("-Werror=deprecated-declarations")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@bcardosolopes bcardosolopes merged commit d5f48dc into llvm:main Jun 26, 2024
6 checks passed
@seven-mile
Copy link
Collaborator

Note that /we4996 may be too aggressive for MSVC.

See the discussion in this issue.

Currently HEAD won't compile with the toolchain of VS2022, which use newer MSSTL with this check introduced in June 2022.

@jopperm
Copy link
Contributor Author

jopperm commented Jun 27, 2024

Thanks for investigating! So until the use of std::complex<APInt|APFloat> is removed upstream or Microsoft changes their STL implementation, we would have three options:

  1. Revert enforcing the check for deprecated API in the build, document this in the coding guidelines, and catch it in code review.
  2. Only do -Werror=deprecated-declarations on Linux (after all, should be enough to check the code on one platform in the CI).
  3. Define _SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING when compiling on Windows to work around the issue.

@jopperm jopperm deleted the isa-cast-dyncast-free-functions branch June 27, 2024 13:14
bcardosolopes pushed a commit that referenced this pull request Jul 3, 2024
Implements approach 2 in
#703 (comment), as
discussed in the community call.

Signed-off-by: Julian Oppermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace uses of isa/dyn_cast/cast/... member functions
3 participants