[PATCH] D116599: Simplify AttrBuilder storage for target dependent attributes

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In D116599#3221724 <https://reviews.llvm.org/D116599#3221724>, @nikic wrote:

> LG from my side, but I'd like a second opinion for the "require LLVMContext in AttrBuilder" part of the change, as that's the main API impact.

I think every non-toy frontend for LLVM probably uses the AttrBuilder API to construct attributes, so it's a pretty broad impact. However, I don't see a good way around it, and I think the improvements to string attribute handling are worth taking the API break.

I will say that the Google was recently affected by the opaque pointer IRBuilder API removal (rG89eb85ac6ea <https://reviews.llvm.org/rG89eb85ac6eab6431ef72ef705d560c72c5ed71f3>), and the ability to pre-migrate code to the new API was very valuable. At least in our usage, where we progressively pick new LLVM versions, it would be helpful to land a no-op AttrBuilder constructor that takes and ignores an LLVMContext. I don't know how common that usage model is for others.


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

https://reviews.llvm.org/D116599