Re: [PATCH] handle attribute positional arguments consistently (PR 87541, 87542)

[Jeff Law <law at redhat dot com>]

On 10/24/18 8:02 PM, Martin Sebor wrote:

>> No camel case.  Make the enum type lower case and its values upper case.
> 
> Done.
> 
> As an aside, I almost thought that after nearly fours years
> I've adjusted to most reviewers' preferences but I'm clearly
> not quite there yet.  As usual, this is not mentioned in
> the coding conventions (except for C++ template parameters
> where it is the preferred spelling), and there are also
> examples of different styles in GCC, including the one
> I chose. For instance, in c-format.c the format_type enum
> uses all lowercase enumerators.  There are also quite a few
> (over a hundred in fact) examples of CamelCase enums in
> various front-ends, back-ends, and other parts of GCC.
It happens.  Avoiding camel case is more important than the upper vs
lower case on the enum constants (and I thought avoiding camel case was
mentioned somewhere, but I could be wrong).  It it wasn't for the  camel
case I probably wouldn't have said anything about the enum constants.

Jeff


> 
>>> @@ -326,17 +331,18 @@ static bool
>>>      }
>>>      }
>>>
>>> -  if (!get_constant (format_num_expr, &info->format_num, validated_p))
>>> -    {
>>> -      error ("format string has invalid operand number");
>>> -      return false;
>>> -    }
>>> +  if (tree val = get_constant (fntype, atname, *format_num_expr,
>>> +                   2, &info->format_num, 0, validated_p))
>>> +    *format_num_expr = val;
>>> +  else
>>> +    return false;
>> Is it really a good idea to be modifying something inside of ARGS like
>> this?  At the very least the function comments neeed updating.
> 
> That's what the code does.  My patch doesn't change it, it just
> adds a new variable.  I added a comment to mention it nonetheless.
Ah.  Must have missed that in the original.  In that case ignore my
comment.  Similarly for the other instance.
.
> 
>>
>>> +{
>>> +  /* Treat zero the same as an out-of-bounds argument number.  */
>>> +  if (!argno)
>>> +    return void_type_node;
>>> +
>>> +  unsigned i = 1;
>>> +
>>> +  for (tree t = TYPE_ARG_TYPES (type); ; t = TREE_CHAIN (t), ++i)
>> There's already iterators to walk over TYPE_ARG_TYPES.  See
>> FOREACH_FUNCTION_ARGS
> 
> As I explained above, I just copied another function just above
> the one I added.
> 
> Besides the one in the function I copied there are six other
> loops in this file and just one use of FOREACH_FUNCTION_ARGS.
> I also think the macro is harder to understand and poor style.
> It requires declaring an extra variable outside the scope of
> the loop even if it isn't used.  So I kept the loop as is.
We're generally trying to use iterators more than open coding loops all
the time.  The existence of code that doesn't use iterators (when
iterators exist) isn't a justification for adding new open coded loops.

Please use the iterator.  OK with that change.

jeff