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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
