martong added a comment. In D80366#2059765 <https://reviews.llvm.org/D80366#2059765>, @martong wrote:
> 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). Just one more thing. It may turn out that the visitation should be done the same way in both cases, then we have to have a Visitor factored out. But, now it seems that even the visitation strategy is different for the individual operations. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80366/new/ https://reviews.llvm.org/D80366 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
