[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

[Aaron Ballman via cfe-commits]

On Fri, Aug 31, 2018 at 9:23 AM, Duncan P. N. Exon Smith via
Phabricator <reviews at reviews.llvm.org> wrote:
> dexonsmith added inline comments.
>
>
> ================
> Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:50
>  // CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
> +// CHECK-NEXT: ExtVectorType (SubjectMatchRule_type_alias)
>  // CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
> ----------------
> aaron.ballman wrote:
>> dexonsmith wrote:
>> > rsmith wrote:
>> > > I think `__attribute__((ext_vector_type))` is a good candidate to *not* have `#pragma clang attribute` support. A type modifier like this really should only be declared as part of declaring the type. Opinions?
>> > The same argument might hold for most type attributes.  I have trouble imagining someone using this, but it's also hard to see how supporting it would lead to bugs in user code.  It seems a bit simpler to support everything.
>> >
>> > I don't have a strong opinion though.
>> I don't think `#pragma clang attribute` should apply to types -- types show up in far too many places and attributes on types changes the fundamental meaning of types a bit too much for my tastes. I'd prefer users annotate the type directly.
>> types show up in far too many places and attributes on types changes the fundamental meaning of types a bit too much for my tastes
>
> I don't see how region-based attributes on type aliases is really any different from region-based annotations on variables.

My reasoning is because type attributes have more impact than variable
attributes and types appear more frequently. Consider using
address_space where the region includes function definitions. Should
that apply to the parameters and return types of the function as well
as the code within the function? Will that be intuitive for users?
What about using a calling convention before defining a structure --
will users expect the calling convention to apply to the member
function types? (The latter is a bit confused though since calling
conventions are declaration *and* type attributes at the same time.)

> Moreover, I'm strongly against disallowing use of this pragma on type aliases in general: as a vendor, we've already shipped that support for a number of attributes.  Most critically, ExternalSourceSymbol applies to type aliases, and handling ExternalSourceSymbol was our primary motivation for adding this feature (the alternative was to add yet-another-attribute-specific-`#pragma`).

ExternalSourceSymbol appertains to named declarations, not types, so
I'm a bit confused where the concern is there.

~Aaron

>
>
> ================
> Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:100
> +// CHECK-NEXT: ObjCReturnsInnerPointer (SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
> +// CHECK-NEXT: ObjCRootClass (SubjectMatchRule_objc_interface)
>  // CHECK-NEXT: ObjCRuntimeName (SubjectMatchRule_objc_interface, SubjectMatchRule_objc_protocol)
> ----------------
> kristina wrote:
>> rsmith wrote:
>> > kristina wrote:
>> > > There is only one root class in a given runtime (ie. NSObject for objc4, or Object for older Apple runtimes, ObjFW also has a different root class). Having more than one makes no sense especially in the same lexical context as there are no use cases I can think of where you would have more than one active ObjC runtime within a process.
>> > Thanks. Yes, this attribute probably doesn't make very much sense to use in conjunction with the pragma.
>> >
>> > So, do we explicitly disallow it, or do we allow it with the expectation that it's likely that no-one will ever want to use it? (That is, do we want to disallow cases that are not useful, or merely cases that are not meaningful?) I don't have a strong opinion here, but I'm mildly inclined towards allowing the pragma on any attribute where it's meaningful, as there may be useful uses that we just didn't think of, and it costs nothing to permit.
>> Yes in theory it could only be used on a single interface in which case it would be perfectly valid. Otherwise when used incorrectly it would issue a diagnostic. As per your inclination of allowing it for all attributes I would say leave it allowed, as it **can** be used in a correct way. Diagnostics are sufficient enough to point out when it happens to apply the attribute to more than one interface.
>>
>> So given your comment, I would say leave it as allowed.
>> So, do we explicitly disallow it, or do we allow it with the expectation that it's likely that no-one will ever want to use it? (That is, do we want to disallow cases that are not useful, or merely cases that are not meaningful?)
>
> I'd prefer to only disallow cases that are not meaningful.
>
>
> Repository:
>   rC Clang
>
> https://reviews.llvm.org/D51507
>
>
>