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>(); ---------------- NoQ wrote: > aaron.ballman wrote: > > NoQ wrote: > > > aaron.ballman wrote: > > > > 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. > > > I confirm the strong local tradition of preferring `auto` for optional > > > `SVal`s in new code, and i believe it's well-justified from the overall > > > coding guideline's point of view. Even if you guys didn't get it right > > > from the start, the amount of Analyzer-specific experience required to > > > understand that it's an optional is very minimal and people get used to > > > it very quickly when they start actively working on the codebase. > > > > > > On one hand, being a class that combines LLVM custom RTTI with > > > pass-by-value semantics (i.e., it's like `QualType` when it comes to > > > passing it around and like `Type` when it comes to casting), optionals > > > are inevitable for representing `SVal` dynamic cast results. > > > > > > On the other hand, `SVal` is one of the most common classes of objects in > > > the Static Analyzer (like, maybe, `Stmt` in Clang; i think a lot of > > > people who are interested in Static Analyzer more than in the rest of > > > Clang learn about `SVal`s earlier than about `Stmt`s), and in particular > > > `SVal` casts are extremely common (around 3-4 `SVal::getAs<T>()` casts > > > per a path-sensitive checker, not counting `castAs`, ~2x more common than > > > arithmetic operations over `SVal`s, only ~3x less common than all sorts > > > of `dyn_cast` in all checkers, including path-insensitive checkers), so > > > it's something you get used to really quickly. > > > > > > Writing `Optional<DefinedOrUnknownSVal> DV = > > > V.getAs<DefinedOrUnknownSVal>();` in a lot of checker callbacks is > > > annoying for pretty much all checker developers from day 1. This clutters > > > the most important parts of the checker's logic: transfer functions and > > > the definition of the error state. Most bugs are in *that* part of the > > > code, and those bugs are *not* due to using `auto` for casts. Not using > > > `auto` almost doubles the amount of code you need to write to perform a > > > simple run-time type check. With a more verbose variable name or a bit > > > more code around it, it also causes a line break. > > > > > > And it only provides information that most of the readers either already > > > know ("`SVal` is a value-type, so it uses optionals"), or memorize > > > immediately after encountering this pattern once on their first day, or > > > barely even care (optionals are used like pointers anyway). Being finally > > > allowed to replace it with just `auto` after migration to C++11 was > > > divine. > > > > > > So i'm still strongly in favor of keeping this pattern included in the > > > list of //"places where the type is already obvious from the context"//. > > > This is the top-1 source of annoying boilerplate in Static Analyzer, and > > > it's as easy to remember as it is to learn that `dyn_cast<T>(S)` returns > > > a pointer (or a reference? - you need to look up the definition of `S` to > > > figure this out, unlike `V.getAs<T>()` that is always an optional for > > > `SVal`s). > > > > > > P.S. Though i admit that coming up with a less annoying API is also a > > > good idea :) > > > P.P.S. I guess some sort of `Optional<auto>` would have made everybody > > > happy. But it's not a thing unfortunately :( > > Thank you for the explanation. However, I'm still not convinced this is > > supported by the coding style guideline. The point to "places where the > > type is already obvious from the context" has (in my estimation) only > > covered places where the type truly is obvious. i.e., it's either spelled > > out explicitly (`dyn_cast<>`, `getAs<>`, etc) or it's entirely immaterial > > for understanding (iterators). Whether a value is optional strikes me as > > highly pertinent information for code reviewers or maintainers to > > understand because it conveys information about the validity of a value. To > > me, this is just as important as the distinction between `auto`, `auto *`, > > and `auto &`, which we make the user spell out in the case of pointers and > > references. From what I can tell, we also spell out `Optional` pretty > > consistently elsewhere in the product. > > > > The rationale that this is a common idiom makes the push-back > > understandable, but at the same time, use of `auto` where the type is not > > immediately obvious makes the static analyzer more hostile to get into, > > especially for people who only stray into this part of the code base > > periodically (which is one major motivation for having a style guide in the > > first place). > > > > I also don't do a ton of work in the static analyzer, so take this feedback > > with a grain of salt. I certainly don't expect to change the static > > analyzer's decision here. However, this is also the second time this week > > I've run into "we don't do that in the static analyzer" and both times have > > been in response to common review feedback that applies elsewhere in the > > product and both times have consumed review time from multiple people in > > order to resolve. > > > > Ultimately, a style guide is just that -- a guide, not a mandate. It may > > also be that deviation from the guide here is reasonable, but it does come > > at a cost. That said, this ship may have already sailed; churning the code > > to remove use of `auto` comes with its own costs that may not be worth > > paying. > Like, i mean, until now i always thought of using `auto` for casts, including > casts of value-types, as of something that the coding guideline explicitly > asks for, and didn't even consider that it might be a deviation, and that's > the first time i hear this concern raised. Though now that you point this > out, the problem with highlighting that it's //the// `llvm::Optional` is > indeed an unclear situation. > > For me as a maintainer, pure `auto` is definitely much easier to read and > review when used with `getAs`. For a casual reader - i don't know, i accept > that it might be harder than i imagine. > > As usual, i'm happy to be proven wrong and/or accept any decision on that > matter. To me coding guidelines are definitely above personal preferences. > > > conveys information about the validity of a value > > This is conveyed via the choice between `getAs` and `castAs`. > Though now that you point this out, the problem with highlighting that it's > the llvm::Optional is indeed an unclear situation. Yup! > This is conveyed via the choice between getAs and castAs. That's a fair point, but in the rest of the code base, `getAs<>` returns a pointer. What started off this discussion was me asking for `const auto *` to clarify that it was a pointer to constant data when it really wasn't one (at least directly). So while it does convey the semantics, it still is a bit confusing. However, I think you're right. The difference between `getAs<>` and `castAs<>` may be sufficient once you know the static analyzer a bit better. https://reviews.llvm.org/D33672 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
