hans added inline comments.
================
Comment at: clang/lib/Sema/SemaCast.cpp:895
+ if (!Self.getLangOpts().RTTIData) {
+ bool isMSVC = Self.getDiagnostics().getDiagnosticOptions().getFormat() ==
+ DiagnosticOptions::MSVC;
----------------
zequanwu wrote:
> 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.
If it's true the casting to void* doesn't need RTTI data (I think it is, but
would be good to verify), then it doesn't make sense to warn. We don't have to
follow MSVC's behavior when it doesn't make sense :)
Similar reasoning for typeid() - I assume it won't work with /GR- also with
MSVC, so warning about it probably makes sense.
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
