[PATCH] D116110: Introduce the AttributeMask class

Duncan P. N. Exon Smith via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 21 13:27:45 PST 2021

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.)



More information about the cfe-commits mailing list