steakhal 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",
----------------
NoQ wrote:
> 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.
Personally, I think we should definitely not branch on the standard version.
This was never a thing anyway, AFAIK.
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
