[llvm-commits] CVS: llvm/include/llvm/ParameterAttributes.h

[Chris Lattner]

>> What is this used for?  I'd expect getParamAttrs to be the main
>> useful api for this class.
>
> Sole client: Bytecode Writer. Its useful in situations where you  
> want to
> traverse the attributes and get the index/attr pairs.

Gotcha, ok.  Please move these methods lower in the class definition  
to de-emphasize them, and add comments saying that clients should use  
the other methods.
>> Did you forget to check in the .cpp file?
>
> No. This is a preview for you to review, which you've done :)

Gotcha :)


> This isn't #included anywhere.

Yep, I know.

>>> +     /// The set of ParameterAttributes set in Attributes is
>>> converted to a
>>> +     /// string of equivalent mnemonics. This is, presumably, for
>>> writing out
>>> +     /// the mnemonics for the assembly writer.
>>> +     /// @brief Convert parameter attribute bits to text
>>> +     static std::string getParamAttrsText(uint16_t Attributes);
>>
>>
>> This requires #include'ing <string>
>
> No, its only #Included into .cpp files, never a .h file. String is
> invariably already included. I can add it if you like, but its
> redundant. For that matter, smallvector probably is too, I didn't  
> check.

Header files should be self contained :).

>>> +     /// The \p Indexth parameter attribute is converted to string.
>>> +     /// @brief Get the text for the parmeter attributes for one
>>> parameter.
>>> +     std::string getParamAttrsTextByIndex(uint16_t Index) const {
>>> +       return getParamAttrsText(getParamAttrs(Index));
>>> +     }
>>
>> I think indexes into the array should be an implementation detail of
>> the class, not exposed to users.
>
> It seems like overkill to make iterators and expose the index/value
> pair.

Sounds fine, I didn't think about the bcwriter.

>>> +     /// @brief Comparison operator for ParamAttrsList
>>> +     bool operator < (const ParamAttrsList& that) const {
>>> +       if (this->attrs.size() < that.attrs.size())
>>> +         return true;
>>> +       if (this->attrs.size() > that.attrs.size())
>>> +         return false;
>>> +       for (unsigned i = 0; i < attrs.size(); ++i) {
>>> +         if (attrs[i].index < that.attrs[i].index)
>>> +           return true;
>>> +         if (attrs[i].index > that.attrs[i].index)
>>> +           return false;
>>> +         if (attrs[i].attrs < that.attrs[i].attrs)
>>> +           return true;
>>> +         if (attrs[i].attrs > that.attrs[i].attrs)
>>> +           return false;
>>> +       }
>>
>> It seems more straight-forward to implement operator< for smallvector
>> and ParamAttrsWithIndex.
>
> Perhaps but this is temporary code. It goes away when FunctionTypes  
> are
> no longer using this to determine type equality. *shrug*

Ok.

>>> +   public:
>>> +     /// This adds a pair to the list of parameter index and
>>> attribute pairs
>>> +     /// represented by this class. No check is made to determine
>>> whether
>>> +     /// param_index exists already. This pair is just added to
>>> the end. It is
>>> +     /// the user's responsibility to insert the pairs wisely.
>>> +     /// @brief Insert ParameterAttributes for an index
>>> +     void insert(uint16_t param_index, uint16_t attrs);
>>
>> I don't like this API.  I think the class should take care of merging
>> attributes for parameters.  Also, should this be named "addAttributes
>> ()" or something?  Logically this is a bucket of attributes, not a
>> sequence, so I don't think 'insert' is a good name.
>
> Okay. If you "add" an attribute does it OR it with existing  
> contents or
> replace existing contents?

It should OR it in.

> It is this way because there are 0 uses of anyone trying to set  
> multiple
> attributes on a given parameter at different points. Generally what is
> done is the bit mask is OR'd together and then insert is called.

RIght.  I'm thinking of things like "pruneeh", which can determine  
that a function never throws.  In that case, it wants to add on the  
"nothrow" attribute.

> Renaming to setAttributes would be acceptable.

"addAttribute" sounds good.  We should probably also have  
'removeAttribute' for symmetry.

>>
>>> +     SmallVector<ParamAttrsWithIndex,2> attrs; ///< The list of
>>> attributes
>>
>> Obviously random/subjective, but I'd suggest bumping this up to
>> having storage for 4 or 6 attributes.
>
> Why? The typical use case that I have seen is 1-2 (sret/sext).  Why do
> you think 4-6 is typical?

I'd expect to see an attribute for every integer argument smaller  
than int, no?

-Chris