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

[Chris Lattner]

> + #ifndef LLVM_PARAMETER_ATTRIBUTES_H
> + #define LLVM_PARAMETER_ATTRIBUTES_H
> +
> + #include <llvm/ADT/SmallVector.h>

Please use ""'s instead of <>'s.

> +   public:
> +     /// Returns the parameter index of a particular parameter  
> attribute in this
> +     /// list of attributes. Note that the attr_index is an index  
> into this
> +     /// class's list of attributes, not the index of the  
> parameter. The result
> +     /// is the index of the parameter.
> +     /// @brief Get a parameter index
> +     uint16_t getParamIndex(unsigned attr_index) const {
> +       return attrs[attr_index].index;
> +     }

What is this used for?  I'd expect getParamAttrs to be the main  
useful api for this class.

> +     /// The parameter attributes for the \p indexth parameter are  
> returned.
> +     /// The 0th parameter refers to the return type of the  
> function. Note that
> +     /// the \p param_index is an index into the function's  
> parameters, not an
> +     /// index into this class's list of attributes. The result of  
> getParamIndex
> +     /// is always suitable input to this function.
> +     /// @returns The all the ParameterAttributes for the \p  
> indexth parameter
> +     /// as a uint16_t of enumeration values OR'd together.
> +     /// @brief Get the attributes for a parameter
> +     uint16_t getParamAttrs(uint16_t param_index) const;

Did you forget to check in the .cpp file?

> +     /// Determines how many parameter attributes are set in this  
> ParamAttrsList.
> +     /// This says nothing about how many parameters the function  
> has. It also
> +     /// says nothing about the highest parameter index that has  
> attributes.
> +     /// @returns the number of parameter attributes in this  
> ParamAttrsList.
> +     /// @brief Return the number of parameter attributes this  
> type has.
> +     unsigned size() const { return attrs.size(); }

What is this useful for?

> +     /// 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>

> +     /// 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.

> +     /// @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.

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

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

Thanks for tackling this Reid!

-Chris