NoQ 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>(); ---------------- 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`. https://reviews.llvm.org/D33672 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
