Re: [PATCH v6] attribs: Implement -Wno-attributes=vendor::attr [PR101940]

[Jason Merrill <jason@redhat.com>]

On 11/9/21 10:55, Marek Polacek wrote:
> On Tue, Nov 09, 2021 at 08:09:10AM +0100, Bernhard Reutner-Fischer wrote:
>> On Tue, 9 Nov 2021 00:12:10 -0500
>> Jason Merrill <jason@redhat.com> wrote:
>>
>>> On 11/8/21 20:41, Marek Polacek wrote:
>>>> On Sat, Nov 06, 2021 at 03:29:57PM -0400, Jason Merrill wrote:
>>
>>>> +  for (auto opt : v)
>>>> +    {
>>>> +      char *cln = strstr (opt, "::");
>>>> +      /* We don't accept '::attr'.  */
>>>> +      if (cln == nullptr || cln == opt)
>>>> +	{
>>>> +	  error ("wrong argument to ignored attributes");
>>>> +	  inform (input_location, "valid format is %<ns::attr%> or %<ns::%>");
>>>> +	  continue;
>>>> +	}
>>>> +      char *vendor_start = opt;
>>>> +      ptrdiff_t vendor_len = cln - opt;
>>>> +      char *attr_start = cln + 2;
>>>> +      /* This could really use rawmemchr :(.  */
>>>> +      ptrdiff_t attr_len = strchr (attr_start, '\0') - attr_start;
>>>> +      /* Verify that they look valid.  */
>>>> +      auto valid_p = [](const char *const s, ptrdiff_t len) {
>>>> +	for (int i = 0; i < len; ++i)
>>>> +	  if (!ISALNUM (*s) && *s != '_')
>>>> +	    return false;
>>>> +	return true;
>>>> +      };
>>>> +      if (!valid_p (vendor_start, vendor_len)
>>>> +	  || !valid_p (attr_start, attr_len))
>>>> +	{
>>>> +	  error ("wrong argument to ignored attributes");
>>>> +	  continue;
>>>> +	}
>>>> +      /* Turn "__attr__" into "attr" so that we have a canonical form of
>>>> +	 attribute names.  Likewise for vendor.  */
>>>> +      auto strip = [](char *&s, ptrdiff_t &l) {
>>>> +	if (l > 4 && s[0] == '_' && s[1] == '_'
>>>> +	    && s[l - 1] == '_' && s[l - 2] == '_')
>>>> +	  {
>>>> +	    s += 2;
>>>> +	    l -= 4;
>>>> +	  }
>>>> +      };
>>>> +      strip (attr_start, attr_len);
>>>> +      strip (vendor_start, vendor_len);
>>>> +      /* We perform all this hijinks so that we don't have to copy OPT.  */
>>>> +      tree vendor_id = get_identifier_with_length (vendor_start, vendor_len);
>>>> +      tree attr_id = get_identifier_with_length (attr_start, attr_len);
>>>> +      /* If we've already seen this vendor::attr, ignore it.  Attempting to
>>>> +	 register it twice would lead to a crash.  */
>>>> +      if (lookup_scoped_attribute_spec (vendor_id, attr_id))
>>>> +	continue;
>>>
>>> Hmm, this looks like it isn't handling the case of previously ignoring
>>> vendor::; it seems we'll look for vendor::<empty string> instead, not
>>> find it, and register again.

> Yes, for -Wno-attributes=vendor::,vendor:: we call register_scoped_attributes
> twice, but I think that's OK: register_scoped_attributes will see that 
> find_attribute_namespace finds the namespace and returns without creating a new  
> one.
> 
> I think the current code handles -Wno-attributes=vendor::a,vendor:: well so
> I'm not sure if I should change it.

Makes sense.  But we should be able to avoid calling 
lookup_scoped_attribute_spec, and even get_identifier_with_length 
(attr...), if attr_len is 0.

>> I don't know if there are __attrib with 2 leading underscores but no
>> trailing that we accept and should normalize?
>>
>> And it is surprising that we do not have a simple normalize_attr_name().
>> There must be existing spots where we would normalize attribute names?
>> extract_attribute_substring does about the same but with a struct substring(?).
>> Maybe that's just to avoid a normal string hash.
> 
> We have canonicalize_attr_name which I based my code on.

We could factor out of that function an overload equivalent to your 
strip lambda?

Jason