NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land.
This looks great to me, I think the patch is good to go as long as Valeriy's comment is addressed :) Speaking of specs, hold up, we're forgetting something very important. `make_unique()` also has to call the //constructor// of the underlying value. We're supposed to model that. The constructor may be inlined or evaluated conservatively. `~unique_ptr()` is also supposed to call the //destructor// of the underlying value. We're supposed to model that. F17422410: aaaa.jpg <https://reviews.llvm.org/F17422410> I.e., you could imagine a bunch of tests like this struct S { int *p; S(int *p): p(p) {} ~S() { *p = 0; } }; void foo() { int x = 1; { auto P = std::make_unique<S>(&x); clang_analyzer_eval(*P->p == 1); // TRUE } clang_analyzer_eval(x == 0); // TRUE } We could stick to conservative evaluation of such constructors and destructors; at the very least, we should actively invalidate Store at the destructor. This patch can remain unchanged because the default values inside freshly conjured regions are unknown. But that'd still be a regression because the above test (presumably) passes with inlining. Is now a good time to legalize modeling function calls inside checker callbacks? ================ Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:3 +// RUN: -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\ +// RUN: -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\ +// RUN: -analyzer-output=text -std=c++20 %s -verify=expected ---------------- Could you double-check that this flag correctly disables all the newly introduced modeling so that it wasn't accidentally enabled for all users before it's ready? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
