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@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
signature.asc
Description: This is a digitally signed message part
_______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
