Szelethus added inline comments.
================ Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs<DefinedOrUnknownSVal>(); ---------------- Szelethus wrote: > ZaMaZaN4iK wrote: > > lebedev.ri wrote: > > > ZaMaZaN4iK wrote: > > > > lebedev.ri wrote: > > > > > ZaMaZaN4iK wrote: > > > > > > lebedev.ri wrote: > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > aaron.ballman wrote: > > > > > > > > > `const auto *` > > > > > > > > Why do we need this change here? If I understand correctly, > > > > > > > > with `const auto*` we also need change initializer to > > > > > > > > `C.getSVal(CE->getSubExpr()).getAs<DefinedOrUnknownSVal>().getPointer()`. > > > > > > > > But I don't understand why we need this. > > > > > > > Is `ValueToCastOptional` a pointer, a reference, or just an > > > > > > > actual `DefinedOrUnknownSVal`? I can't tell. > > > > > > > (sidenote: would be great to have a clang-tidy check for this.) > > > > > > `ValueToCastOptional` is `llvm::Optional<DefinedOrUnknownSVal>` > > > > > See, all my guesses were wrong. That is why it should not be `auto` > > > > > at all. > > > > I don't agree with you for this case. Honestly it's like a yet another > > > > holywar question. If we are talking only about this case - here you can > > > > see `getAs<DefinedOrUnknownSVal>` part of the expression. this means > > > > clearly (at least for me) that we get something like > > > > `DefinedOrUnknownSVal`. What we get? I just press hotkey in my > > > > favourite IDE/text editor and see that `getAs` returns > > > > `llvm::Optional<DefinedOrUnknownSVal>`. From my point of view it's > > > > clear enough here. > > > > > > > > If we are talking more generally about question "When should we use > > > > `auto` at all? " - we can talk, but not here, I think :) > > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable > > > comes to mind. > > > > What we get? I just press hotkey in my favourite IDE/text editor and > > > > see that getAs returns llvm::Optional<DefinedOrUnknownSVal> > > > Which hotkey do i need to press to see this here, in the phabricator? > > > > > > This really shouldn't be `auto`, if you have to explain that in the > > > variable's name, justify it in review comments. > > Ok, didn't know about such LLVM coding standard. Of course, with this > > information I will fix using `auto` here. Thank you. > Actually, I disagree. In the Static Analyzer we use `auto` if the return type > is in the name of the expression, and `getAs` may fail, so it returns an > `Optional`. In the case where a nullptr may be returned to signal failure, > `auto *` is used, so I believe that `auto` is appropriate here. But don't change it back now, it doesn't matter a whole lot :D https://reviews.llvm.org/D33672 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
