[PATCH] D136526: [AAPointerInfo] refactor how offsets and Access objects are tracked

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:832
+  // information in OffsetAndSize in the Access object.
+  SmallVector<AAPointerInfo::Access, 4> AccessList;
+  OffsetBinsTy OffsetBins;
----------------
sameerds wrote:
> jdoerfert wrote:
> > We could use Access* here to get stable addresses.
> We could, but then we have to worry about memory allocation on the stack. In fact, I am still wondering about the copy constructor for PointerInfo::State. It seems to make a shallow copy of AccessBins. Doesn't that result in a double free? 
> 
> If we keep a list of Access*, instead of a list of Access, it looks like more code with no clear benefit. TBH, I am a big fan of "never use new". For now, the unsigned index into the list of Access is sufficient for its very local use. If it needs to be exposed to clients, maybe could wrap it in a custom iterator instead of exposing stable pointers?
> 
> Of course I am open to using a list of Access* if you think that is beneficial.
s/stack/heap


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136526