NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:157
"Storage provided to placement new is only {0} bytes, "
- "whereas the allocated array type requires more space for "
- "internal needs",
- SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+ "whereas the allocated array type might require more space for "
+ "allocation overhead",
----------------
isuckatcs wrote:
> martong wrote:
> > NoQ wrote:
> > > "might" is not very convincing, it may cause a reaction like "I've no
> > > idea what it's talking about and the compiler itself isn't sure, must be
> > > false positive". Can we do anything to point the user in the right
> > > direction? Say, if this is implementation-defined, are we looking at a
> > > portability issue?
> > Okay, I see your point. Let's dissect the corresponding sections of the
> > standard:
> > ```
> > new(2, f) T[5] results in a call of operator new[](sizeof(T) * 5 + y, 2, f).
> >
> > Here, ... and y are non-negative unspecified values representing array
> > allocation overhead;
> > ```
> > The array overhead is an **unspecified value**. What does it mean exactly?
> > My understanding is that this means it is implementation defined and the
> > implementation is not required to document or guarantee anything. I came to
> > this conclusion based on this [[
> > https://stackoverflow.com/questions/2397984/undefined-unspecified-and-implementation-defined-behavior
> > | stackoverflow question ]].
> >
> > My interpretation could be wrong, but that does not matter. I think, we
> > should just simply display the user what the standard says, and let them
> > digest themselves the meaning of "unspecified". I am updating the patch
> > like so.
> I also looked into this overhead question. In this [[
> https://stackoverflow.com/a/8721932 | stackoverflow answer]] someone links
> the same defect report that you did, and claims that since it's a defect
> report, it has been applied retroactively.
>
> Apparently [[ https://en.cppreference.com/w/cpp/language/new | cppreference
> ]] says the same. If you check the defect reports section on the bottom of
> the page you can see:
> ```
> The following behavior-changing defect reports were applied retroactively to
> previously published C++ standards.
>
> ...
> DR | Applied to | Behavior as published | Correct behavior
> CWG 2382 | C++98 | non-allocating placement array new could require
> allocation overhead | such allocation overhead disallowed
> ```
>
> So in my understanding you don't need to worry about this overhead at all. I
> hope this helps.
Ok it sounds like we shouldn't emit any warnings at all here, unless we're
somehow able to get a signal that the user is compiling their code with a
pre-2019 compiler that hasn't been updated to reflect this defect report. One
way we could get that signal is by putting the checker into the `portability`
package which is off-by-default. Then we can explain the situation in the
warning message:
> "Storage provided to placement new is only {0} bytes, which is sufficient to
> hold the array data but placement new may require additional unspecified
> array allocation overhead on compilers that don't implement CWG 2382"
Then if the users enable `portability` but aren't interested in this warning,
at least they know how to enable and disable checkers, so they can disable this
specific check.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129280/new/
https://reviews.llvm.org/D129280
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
