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: > > 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. 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