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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
