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

[Martin Sebor]

On 10/08/2018 07:27 AM, Manuel López-Ibáñez wrote:

On 07/10/18 23:38, Martin Sebor wrote:

+  pretty_printer posval;
+  if (pos != error_mark_node)
+    {
+      /* Only format the position value when it's valid.  By convention
+     do not quote constant integers.  */
+      pp_space (&posval);
+      if (TREE_CODE (pos) != INTEGER_CST)
+    pp_begin_quote (&posval, pp_show_color (global_dc->printer));
+      dump_generic_node (&posval, pos, 0, TDF_NONE, false);
+      if (TREE_CODE (pos) != INTEGER_CST)
+    pp_end_quote (&posval, pp_show_color (global_dc->printer));
+    }

Sorry for the bike-shedding but is this really necessary?

First, you handle != INTEGER_CST separately, so you can simply use %qE
for that case and %E for the rest. Nevertheless, I think the convention
is (https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Quoting): "elements
such as numbers that do no refer to numeric constants that appear in the
source code should not be quoted". Since this is a integer constant that
appears in the source code, then it should be quoted.

Ah, I forgot about this.  I'm happy to quote it.

Also, "value%s" where %s can be empty, will not translate correctly.

Why not?  (I had thought about it but convinced myself it would
not be a problem, but it's certainly possible I missed something.)

Perhaps you need a small function:

warn_attributes(const char * msg_no_value_no_arg,
                const char * msg_no_value_arg,
        const char * msg_value_no_arg,
                const char * msg_value_arg,
        tree atname, int argno, tree pos, ...)

+  if (TREE_CODE (pos) != INTEGER_CST)
+    {
+      /* Only mention the argument number when it's non-zero.  */
+      if (argno < 1)
+    warning (OPT_Wattributes,
+         "%qE attribute argument value%s is not an integer "
+         "constant",
+         atname, pp_formatted_text (&posval));
+      else
+    warning (OPT_Wattributes,
+         "%qE attribute argument %i value%s is not an integer "
+         "constant",
+         atname, argno, pp_formatted_text (&posval));
+
+      return NULL_TREE;
+    }

So that in the code above you can say:

if (TREE_CODE (pos) != INTEGER_CST)
{
warn_attributes("%qE attribute argument value is not an integer",
        "%qE attribute argument %i value is not an integer",
        "%qE attribute argument value %qE is not an integer",
        "%qE attribute argument %i value %qE is not an integer",
                 atname, argno, pos);
return NULL_TREE;
}

Thanks.  If it it turns out I'm wrong about the translation I'll
see if I can use something like it.  The concern raised with this
approach in the past was that GCC's -Wformat can't check the format
strings but I can't think of a better way.

Also, I wonder where input_location is pointing at for these warnings.
There may be a better location. Clang is doing:

<source>:5:1: error: 'alloc_align' attribute requires parameter 1 to be
an integer constant
ALIGN ("1") void*
^      ~~~
<source>:1:36: note: expanded from macro 'ALIGN'
#define ALIGN(N)   __attribute__ ((alloc_align (N)))
                                   ^            ~

while GCC does:

<source>:6:16: warning: alloc_align parameter outside range [-Wattributes]
6 | fpvi_str_1 (int);
  |                ^

Unfortunately, there is no location information associated with
attribute arguments -- they are locationless constants -- so
the warnings point at (or just past) the current function
declaration.  David would know how difficult it might be to
add it to make the diagnostics look better.

Apart from the above, this seems a major improvement, so I hope it goes in.

Thanks
Martin