NoQ added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:32 + const MemRegion *BaseRegion = MRegion->getBaseRegion(); + assert(BaseRegion == Offset.getRegion()); + ---------------- martong wrote: > This assertion fails on real code quite often (e.g. on LLVM/Clang code). I > don't really understand why. @NoQ what is you understanding on this? Perhaps > I could try to reduce a case from the real code to see an example. `RegionOffset` cannot really represent symbolic offsets :( You should be able to re-add the assertion after checking for symbolic offsets. And, yeah, you can always easily reduce any crash with `creduce`. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:22 + ProgramStateRef State) const; + mutable std::unique_ptr<BuiltinBug> BT_Placement; +}; ---------------- martong wrote: > xazax.hun wrote: > > I think now it is safe to have the bugtype by value and use member > > initialization. > Ok, I've made it to be a simple member. Also could remove the `mutable` > specifier. And use a default initializer instead of constructor >.> `BuiltinBug BT_Placement{this, "Insufficient storage BB"}` (also i don't understand `BuiltinBug` and i'm afraid of using it, can we just have `BugType` with explicit description and category?) ================ Comment at: clang/test/Analysis/placement-new.cpp:11 +void f() { + short s; // expected-note-re {{'s' declared{{.*}}}} + long *lp = ::new (&s) long; // expected-warning{{Argument of default placement new provides storage capacity of 2 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 3 {{}} ---------------- martong wrote: > NoQ wrote: > > I'm legit curious what's hidden behind the regex. > Ok, I removed the regexes. > But keep in mind that, this note is coming from `trackExpressionValue` and is > changing if the initializer of `s` is changing. > I did not want to formulate expectations on a note that is coming from an > implementation detail and can change easily. Also, If `trackExpressionValue` > changes than we need to rewrite all these tests, unless a regex is used, > perhaps just a simple expected-note {{}} would be the best from this point of > view. Because it's up to the checker whether to use `trackExpressionValue` or provide its own visitor, i'd rather keep the notes tested. If the notes provided by `trackExpressionValue` aren't good enough, it's ultimately the checker's problem (which may or may not be fixed by improving `trackExpressionValue`). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71612/new/ https://reviews.llvm.org/D71612 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits