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
