dexonsmith added a comment. In D116110#3205416 <https://reviews.llvm.org/D116110#3205416>, @nikic wrote:
> This looks good to me, but I'd recommend waiting a bit before landing in case > there's further input, as this is a non-trivial API change. (Maybe valuable to start a quick thread on llvm-dev to get more eyes on this? (I'm not suggesting that's required, just pointing out the option...)) > I think separating AttrBuilder (which stores attribute keys and values) from > AttributeMask (which only stores keys) makes sense conceptually. With the > current API, it's not really obvious that the attribute values are completely > ignored for removal operations -- on the surface it looks like the attribute > would only get removed if both key and value match, but that's not how it > actually works. > > This should also give more flexibility in optimizing AttrBuilder for it's > primary use case (which is adding attributes). FWIW, I agree that this approach seems good. (SGTM, not LGTM, only because I haven't reviewed the patch details.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116110/new/ https://reviews.llvm.org/D116110 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
