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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
