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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.


================
Comment at: clang/lib/Sema/SemaCast.cpp:895
+  if (!Self.getLangOpts().RTTIData) {
+    bool isMSVC = Self.getDiagnostics().getDiagnosticOptions().getFormat() ==
+                  DiagnosticOptions::MSVC;
----------------
hans wrote:
> I'm not sure isMSVC is the best name (it's not clear what aspect is MSVC exactly -- in this case it's the diagnostics format).
> 
> It's possible to target MSVC both with clang-cl and with regular clang.
> 
> For example, one could use
> 
>   clang-cl /c /tmp/a.cpp
> 
> or
> 
>   clang -c /tmp/a.cpp -target i686-pc-windows-msvc19.11.0 -fms-extensions
> 
> 
> My understanding is that the purpose of "isMSVC" here is to try and detect if we're using clang-cl or clang so that the diagnostic can say "/GR-" or "-fno-rtti-data". So maybe it's better to call it "isClangCL" or something like that.
> 
> Also, I don't think we should check "isMSVC" in the if-statement below. We want the warning to fire both when using clang and clang-cl: as long as -fno-rtti-data or /GR- is used, the warning makes sense.
> 
> So I think the code could be more like:
> 
> ```
> if (!Self.getLangOpts().RTTIData && !DestPointee->isVoidType()) {
>   bool isClangCL = ...;
>   Self.Diag(...) << isClangCL;
> }
> ```
MSVC will warn even if the DestPointee is void type. What I thought is if invoked by clang-cl warn regardless of DeskPointee type. If invoked by clang, warn if it's not void type. 
https://godbolt.org/z/475q5v. I noticed MSVC won't warn at typeid if /GR- is given. Probably I should remove the warning in typeid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86369