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

[Aaron Ballman via Phabricator via cfe-commits]

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

The static analyzer should be following the same coding standard as the rest of the project. This is new code, so it's not matching inappropriate styles from older code, so there's really no reason to not match the coding standard.

> In the case where a nullptr may be returned to signal failure, auto * is used, so I believe that auto is appropriate here.

I disagree that `auto` is appropriate here. The amount of confusion about the type demonstrates why.


https://reviews.llvm.org/D33672