[PATCH] D151325: [analyzer] Differentiate lifetime extended temporaries

Thu May 25 22:26:26 PDT 2023

tomasz-kaminski-sonarsource added a comment.

In D151325#4374326 <https://reviews.llvm.org/D151325#4374326>, @NoQ wrote:

> I totally support this! There's no reason why the region wouldn't carry such information.
>
> This patch is surprisingly tiny, but this matches my grep observations: almost nothing in the static analyzer treats `CXXTempObjectRegion` specially. Even in case of live symbols/regions analysis, it simply relies on how temporary region is bound in the Store to the lifetime-extending reference, so it doesn't need to be handled explicitly.

Indeed. However, I think that this could be a side product of `CXXTempObjectRegion` not representing actual short lived temporaries.

> I see that there's no change in `MoveChecker` which treats `CXXTempObjectRegion` specially, and I suspect it may start emitting more warnings after this patch, but I'm not sure if it's a good thing or a bad thing. Could be worth experimenting (eg. by analyzing LLVM itself, it has enough move semantics to demonstrate potential problems).

I have performed our internal test, which includes LLVM, and it has no changes in the results due to this patch. 
For the `MoveChecker`, after inspecting the code I believe that we should exclude only `CXXTempObjectsRegions` from that checker, as it is certainly possible to perform operations on moved from lifetime extended object.

> Do you have any preferences on creating a common base class for `CXXTempObjectRegion` and `CXXLifetimeExtendedObjectRegion`? Or, as an opposite extreme, simply add a nullable `ExD` field to `CXXTempObjectRegion` instead of making a new class? Both could save us some code when these regions do need to be treated uniformly, but it doesn't look like a popular situation to worry about.

Despite them, both being referred to as temporary, they are very different in nature. Thus I believe they should be considered separately by each checker, thus I prefer them to not be related. This is why I skipped `Temp` word in the name of the region.
If any, I would group them with `VarRegion`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151325