[PATCH] D28266: Transparent_union attribute should be possible in front of union (rework)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Some minor nits, but I think this is generally correct.



================
Comment at: include/clang/Sema/Sema.h:3059
   void ProcessDeclAttributes(Scope *S, Decl *D, const Declarator &PD);
+  // Helper for delayed proccessing TransparentUnion attribute.
+  void ProcessDeclAttributeDelayed(Decl *D, const AttributeList *AttrList);
----------------
I would remove the mention of TransparentUnion -- this is generalized for other uses already.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:1889
+  if (!TagOrTempResult.isInvalid())
+    // Delayed proccessing TransparentUnion attribute.
+    Actions.ProcessDeclAttributeDelayed(TagOrTempResult.get(), attrs.getList());
----------------
I'd remove mention of TransparentUnion here as well.


================
Comment at: lib/Sema/SemaDecl.cpp:13499
+  if (Attr)
+    ProcessDeclAttributeList(S, New, Attr);
+
----------------
erichkeane wrote:
> I wasn't sure if this was the correct change in here.  My other option was to move the above 2 lines (13495/13496) above these line's original spot, but moving it here seems to have correctly passed all tests.  Anyone with more knowledge here want to chime in?
I think this change is reasonable.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:6201
+void Sema::ProcessDeclAttributeDelayed(Decl *D, const AttributeList *AttrList) {
+  for (const AttributeList* l = AttrList; l; l = l->getNext())
+    if (l->getKind() == AttributeList::AT_TransparentUnion) {
----------------
Nit: the `*` should bind to `l` and `l` should be `L` by our usual naming conventions (but a name better than `L` wouldn't be amiss).


https://reviews.llvm.org/D28266