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:
> zequanwu wrote:
> > hans wrote:
> > > 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.
> > In clang, I believe that dynamic_cast to void* doesn't use RTTI data:
> > https://godbolt.org/z/Kbr7Mq
> > Looks like MSVC only warns if the operand of typeid is not pointer:
> > https://godbolt.org/z/chcMcn
> >
> When targeting Windows, dynamic_cast to void* is implemented with in a
> runtime function, RTCastToVoid: https://godbolt.org/z/Kecr7z
> I wonder if that uses RTTI data internally though...
>
> For typeid() I guess it would also warn on references? Maybe we should do the
> same.
Couldn't find if `__RTCastToVoid` uses RTTI data internally.
For typeid(), it also warn on references. But the behavior is a bit weird
(https://godbolt.org/z/jn4Pjx). Seems like it warns only when dereferencing a
pointer or argument is a reference.
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
