[PATCH] D19181: Map Attribute in the C API.

[James Y Knight via llvm-commits]

jyknight added a comment.

In http://reviews.llvm.org/D19181#414320, @deadalnix wrote:

> I don't think exposing AttrBuilder is a wise move. I don't think exposing the attribute set is either. This is not a super convenient API to begin with, proof is the amount of accessors that wrap it all over the place in the C++ code. I'll have to side with @whitequark here.


I agree, let's not add AttrBuilder unless there was shown a real need for it. But then let's not talk about efficiency as a primary goal here, either.

To me, a minimal yet usable and fully functional API seems like it should be the goal.

So, I don't understand why you'd expose LLVMAttributeRef. That actually feels much more like an internal implementation detail than the AttributeSet as a whole. I kind of like exposing the AttributeSet, as it's a reusable object which you can easily assign to multiple instructions without rebuilding from scratch each time.

However, if we don't want to expose AttributeSet (which, ok, perhaps is the right call here), I'd make some small changes to my proposal:

1. Remove LLVMGetAttrs and LLVMSetAttrs.
2. Modify all of the functions "LLVMAS*" that took an LLVMAttributeSetRef, to instead take an LLVMValueRef of the function or call instruction directly. (and rename the functions to something more appropriate).

That leaves, most importantly, some properties of the original proposal:

- The attribute location, function vs return vs param, is a parameter, not part of the function name.
- The same function works on functions and call/invoke instructions, which after all, are both exposed as LLVMValueRef.
- All types (string, int, on/off flag) of attributes are accessible in all locations.


http://reviews.llvm.org/D19181