Re: [PATCH] include/linux/compiler*.h: Use feature checking instead of version checks for attributes

[Miguel Ojeda]

Hi Nick,

Actually, to acknowledge the comments to the other email...

On Tue, Aug 28, 2018 at 6:53 PM, Nick Desaulniers
<ndesaulniers@xxxxxxxxxx> wrote:
> On Tue, Aug 28, 2018 at 8:04 AM Miguel Ojeda
> <miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>>
>> Hi Nick,
>>
>> On Mon, Aug 27, 2018 at 7:43 PM, Nick Desaulniers
>> <ndesaulniers@xxxxxxxxxx> wrote:
>> > On Sun, Aug 26, 2018 at 10:58 AM Miguel Ojeda
>> > <miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>> >>
>> >> Instead of using version checks per-compiler to define (or not) each attribute,
>> >> use __has_attribute to test for them, following the cleanup started with
>> >> commit 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive").
>> >>
>> >> All the attributes that are fairly common/standard (i.e. those that do not
>> >> require extra logic to define them) have been moved to a new file
>> >> include/linux/compiler_attributes.h. The attributes have been sorted
>> >> and divided between "required" and "optional".
>> >
>> > Nice! Thanks Miguel.  Regarding sorting, I'm happy with that.  In
>> > fact, some of the comments can be removed IMO, as the attributes have
>> > common definitions in the docs (maybe an added link to the gcc and
>> > clang attribute docs at the top of the file rather than per attribute
>> > comments).
>>
>> Thanks for the review!
>>
>> I thought about that, although there isn't a single page with them in
>> GCC (we could group them by type though: function ones, variable
>> ones... and then link to those).
>> On the other hand, maybe writing a
>> Doc/ file is better and allows us to write as much as one would like
>> about each of them (and a link to each page compiler's page about it,
>> etc.). I think in the end the Doc/ file might be the best, in order
>> not to crowd the header.
>
> A comment is closer to the source, but I guess that's bytes for each
> inclusion for every file.  I don't feel passionate about this point
> one way or the other.
>

I think I will write a simple Doc/ file, link to it from the source,
and see if people like it.

>>
>> >
>> >>
>> >> Further, attributes that are already supported in gcc >= 4.6 and recent clang
>> >> were simply made to be required (instead of testing for them):
>> >>   * always_inline
>> >>   * const (pure was already "required", by the way)
>> >>   * gnu_inline
>> >
>> > There's an important test for gnu_inline that isn't checking that it's
>> > supported, but rather what the implicit behavior is depending on which
>> > C standard is being used.  It's important not to remove that.
>>
>> Hm... I actually thought it was not available at some point before 4.6
>> and removed the #ifdef. The comment even says it is featuring
>> detecting it so that the old GCC inlining is used; but it shouldn't
>> matter if you always use it, no?
>
> Good point.  Rather than defining it only if GNU inline is not the
> current behavior is a bit more verbose than just always defining it.
> This seems to confirm that that should work:
> https://godbolt.org/z/igwh32.
>

Great then! Thanks for confirming!

Cheers,
Miguel