[PATCH] D129280: [analyzer] PlacementNewChecker, properly handle array overhead (cookie)

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs 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",
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129280/new/

https://reviews.llvm.org/D129280