llvm / clangir

A new (MLIR based) high-level IR for clang.
https://clangir.org
Other
307 stars 84 forks source link

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

Closed jopperm closed 1 week ago

jopperm commented 1 week ago

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:

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 6b7420a93278ee01d37d95882dec39358378cfb3); 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).

seven-mile commented 1 week ago

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 commented 1 week ago

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.