[PATCH] D80366: [Analyzer] Add `getReturnValueUnderConstruction()` to `CallEvent`

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In this version of the patch you are using `handleConstructionContext` to "retrieve" or read an SVal for the construction. However, it seems like that function was designed to store values in the GDM for individual construction context items. To mix a read-only functionality into a setter seems to be error-prone to me.

> Please instead re-use the code that computes the object under construction. That'll save you ~50 lines of code and will be more future-proof

I am not sure that this is the best option here. In my viewpoint, ConstructionContext and ConstructionContextItem are both "variants" (i.e. tagged unions) and we perform operations on these. Each operation is actually a visitation on the different kind of values (i.e. a switch). One operation is transforming a ConstructionContext to a ConstructionContextItem. This is what is needed to be able to call the getObjectUnderConstruction function. And this operation - the reading - is way different than the other - the store -, see Adam's concerns. This yields to different functions for each op. But this is perfectly natural once we work with "variants". I mean we can easily extend the operations on a variant with a new operation as a form of a visitation of the kinds (contrary to traditional class hierarchies where adding a new op is hard, but adding a new type is easy).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80366/new/

https://reviews.llvm.org/D80366