[PATCH] D81718: [Analyzer][NFC] Add methods `getReturnObject()` and `getArgObject()` to `CallEvent`

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Thank you for taking a look on this!

Let me explain! An iterator may be implemented by multiple ways. It can be a simple pointer, which means that it is a basic type passed by value both as argument and return value. Thus here `getArgSVal()` and `getReturnValue()` are the correct methods to invoke. It can be something small (e.g. a pointer) wrapped into a class. It is also passed by value both as argument (maybe also for comparison operators ) and return value, but since it is a class instance, both require copy construction. This means that here `getParameterLocation()` and `getReturnValueUnderConstruction()` are the right methods. Furthermore, it can be a somewhat larger class that is passed by constant reference as argument, but of course by value as return value. In this case `getArgSVal()` and `getReturnValueUnderConstruction()` are the winners. Also the same class can be a value parameter in one function and a reference parameter in another one. The checker does not know this in advance, but must work correctly for all these cases.

In this patch `getReturnObject()` uses a "trial and failure" approach, thus it tries to assume that we are handling a class instance, which is copy constructed, so it tries to retrieve it from the construction context of the call. If no such context exists then it means that the value is not copy constructed. I think this approach is correct.

On the other hand, I agree with you that in `getArgObject()` checking the `SVal` is insufficient/incorrect. (I also planned to make a self-comment there, but I forgot.) Is it OK, if I look at the callee instead, and examine the type of its parameter? If it is passed by value, and is a `C++` class instance, then I use `getParameterLocation()`, otherwise `getArgSVal()`. Of course, I can do the same for `getReturnObject()`, but the result is exactly the same as the current "trial and failure" approach.

It is also possible to put these functions into `Iterator.h`, but I think that other checkers may also benefit from such functionality. One thing is sure: I need such wrapper functions somewhere, without them all the iterator-related checkers will be unreadable because of the code multiplication. There are lots of places where we retrieve one of the arguments or the return value, and checking the nature of the value everywhere almost duplicates the size of the code and makes it unreadable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81718