>> 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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
