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:
> > > > > 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.
> Okay, maybe it's safest to warn about dynamic_cast also for void*, like MSVC
> does.
>
> For typeid() maybe we should be conservative like Clang is with -fno-rtti,
> and always warn.
>
> But I'm a little bit surprised about this, because I'd imagine typeid(int)
> could work also with -fno-rtti... if you're curious maybe it would be
> interesting to dig deeper into how this works.
It might just because typeid is a rtti-only thing when introduced,
https://en.cppreference.com/w/cpp/types. gcc also gives error for this.
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
