[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:895
+  if (!Self.getLangOpts().RTTIData) {
+    bool isMSVC = Self.getDiagnostics().getDiagnosticOptions().getFormat() ==
+                  DiagnosticOptions::MSVC;
----------------
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;
}
```


================
Comment at: clang/test/SemaCXX/ms_no_dynamic_cast.cpp:1
+// RUN: %clang_cl %s /GR- -fsyntax-only 2>&1 | FileCheck %s
+
----------------
zequanwu wrote:
> hans wrote:
> > When using %clang_cl, the source file should always come after a "--", otherwise if for example the source file is "/Users/foo/src/test.cc" the filename can get interpreted as a command-line option.
> > 
> > But tests outside of Driver/ generally invoke cc1 directly, with %clang_cc1 and passing the appropriate flags. I'd suggest doing that here too. (And in the other test.)
> This test is for testing in clang-cl. Another test is for clang. If I use %clang_cc1, /GR- can not be passed.
But clang-cl is just a driver mode, all it does is pass flags to the cc1 invocation. So normally we test the driver separately (in Driver/) and the compiler (cc1) separately.

In this case the warning lives in the compiler (cc1) so it makes sense to target that directly with the test. If you grep through clang/test/Sema and clang/test/SemaCXX, you'll see that %clang_cl is not use in any test.

You should be able to run cc1 with and without "-fdiagnostics-format msvc" and -fno-rtti-data to test the new warnings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86369