[PATCH] D86369: [Sema][MSVC] warn at dynamic_cast when /GR- is given

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.


================
Comment at: clang/lib/Sema/SemaCast.cpp:897
+                  DiagnosticOptions::MSVC;
+    if (isMSVC || !DestPointee->isVoidType())
+      Self.Diag(OpRange.getBegin(),
----------------
zequanwu wrote:
> hans wrote:
> > I don't think the if-statement is necessary. I thought we concluded we want to warn even for void*? Also note that "isMSVC" here is only checking the driver mode, i.e. just the user interface to the compiler. The user could still be compiling in MSVC mode.
> My bad. I thought you meant to use the previous version, so I reverted this region.
> Like we concluded, we want to warn even for void*. This only applies to clang-cl, not clang, right? This is the purpose of this if. If it's in clang-cl, warn regardless of destination type. If it's in clang, don't warn for void*, like line 887 which doesn't emit error for void* destination type. 
> 
> If RTTI is false, RTTIData is also false (https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/CompilerInvocation.cpp#L2870). There is a test https://github.com/llvm/llvm-project/blob/master/clang/test/SemaCXX/no-rtti.cpp#L28, which should not be warned, right?
> Like we concluded, we want to warn even for void*. This only applies to clang-cl, not clang, right?

Oh, right, we need to do the "even for void*" part only when using the Microsoft ABI. It's not about clang or clang-cl, those are just different user interfaces really, but about what platform (processor, abi, etc.) the compiler is targeting. It's possible to target Windows with regular clang, not just clang-cl.

Inside Sema, you can check Context.getTargetInfo().getCXXABI().isMicrosoft() to see if the microsoft abi is being targeted.

Apologies for my confusing comments here.


================
Comment at: clang/test/SemaCXX/ms_no_dynamic_cast.cpp:19
+  auto d = dynamic_cast<D1 *>(b); // expected-warning{{dynamic_cast will not work since RTTI data is disabled by /GR-}}
+  (void)typeid(int);              // expected-warning{{typeid will not work since RTTI data is disabled by /GR-}}
+}
----------------
zequanwu wrote:
> hans wrote:
> > Is the cast to void necessary? Same comment for the next file.
> Yes, to suppress the warning of unused expression.
Ah, okay. Didn't realize that warning was on by default :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86369/new/

https://reviews.llvm.org/D86369