[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri 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>();
----------------
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.


https://reviews.llvm.org/D33672