[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/include/clang/Basic/DiagnosticSemaKinds.td:7436
   "use of dynamic_cast requires -frtti">;
+def warn_no_dynamic_cast_with_RTTI_disabled: Warning<
+  "dynamic_cast will not work since RTTI data is disabled by " 
----------------
The other warning names spell rtti in lower-case, so I'd suggest that here too for consistency.


================
Comment at: clang/lib/Sema/SemaCast.cpp:895
+  if (!Self.getLangOpts().RTTIData &&
+      !Self.getDiagnostics().isIgnored(diag::warn_no_typeid_with_RTTI_disabled,
+                                       OpRange.getBegin())) {
----------------
Is the isIgnored() check necessary here? I think in general we call Diag() anyway, and if the warning is ignored, it will be ignored.

The reason we sometimes check isIgnored() explicitly is to avoid expensive computation when possible, but that's not the case here.


================
Comment at: clang/lib/Sema/SemaCast.cpp:900
+                diag::warn_no_dynamic_cast_with_RTTI_disabled)
+          << Self.getLangOpts().MSVCCompat;
+  }
----------------
getLangOpts().MSVCCompat can be true both with clang-cl and regular clang, it just depends on teh -fms-compatibility flag.

Driver::IsCLMode() is the sure way to check for clang-cl, but I don't think that information is forwarded to the compiler frontend.

One slightly hacky way to check, and in this case maybe it makes sense, is to look at the TextDiagnosticFormat. If that's MSVC, it's very likely the driver was clang-cl.


================
Comment at: clang/test/SemaCXX/ms_no_dynamic_cast.cpp:1
+// RUN: %clang_cl %s /GR- -fsyntax-only 2>&1 | FileCheck %s
+
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86369