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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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.

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`).


================
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