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

[Reid Spencer]

On Sun, 2007-04-08 at 14:29 -0700, Chris Lattner wrote:
> > + #ifndef LLVM_PARAMETER_ATTRIBUTES_H
> > + #define LLVM_PARAMETER_ATTRIBUTES_H
> > +
> > + #include <llvm/ADT/SmallVector.h>
> 
> Please use ""'s instead of <>'s.

Okay.

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

Sole client: Bytecode Writer. Its useful in situations where you want to
traverse the attributes and get the index/attr pairs.

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

No. This is a preview for you to review, which you've done :)

This isn't #included anywhere.

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

Bytecode writer.

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

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

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

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

Renaming to setAttributes would be acceptable.

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

> 
> Thanks for tackling this Reid!
> 
> -Chris
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070408/cb7ac0da/attachment.sig>