[AST] Fix compilation with GCC < 8 for MinGW

[mstorsjo]

Summary

GCC 7 and earlier, when targeting MinGW, seems to have a bug in layout/size of bitfield structs if they contain a nested enum, making the size of the struct 8 bytes, while we have a static assert requiring it to be 4 bytes or less.

While this clearly is a GCC bug, the workaround (moving the enum out of the bitfield) also is very nonintrusive and matches other existing enums there.

Testcase:

$ cat bitfield.cpp
struct MyStruct {
  unsigned a : 1;
  enum { SomeValue = 42 };
  unsigned b : 1;
};      
int StructSize = sizeof(struct MyStruct);
$ x86_64-w64-mingw32-g++ -S -o - bitfield.cpp | grep -C 2 StructSize 
        .file   "bitfield.cpp"
        .text
        .globl  StructSize
        .data
        .align 4
StructSize:
        .long   8 
        .ident  "GCC: (GNU) 7.3-win32 20180312"

[rjmccall]

Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM.

[mstorsjo]

In D71650#1789897, @rjmccall wrote:

Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM.

The post-patch form doesn't look that odd to me (and we wouldn't want one comment for every one of the existing enums that already are outside of the structs where they are used for bitfield sizes), but do you think a comment is warranted here on this one?

[rjmccall]

In D71650#1790067, @mstorsjo wrote:

In D71650#1789897, @rjmccall wrote:

Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM.

The post-patch form doesn't look that odd to me (and we wouldn't want one comment for every one of the existing enums that already are outside of the structs where they are used for bitfield sizes), but do you think a comment is warranted here on this one?

I think having a comment somewhere up there to say clearly that these things are hoisted up there as a workaround for a specific GCC port might help somebody from making this mistake again. It also might not, but certainly not having a comment can't help.

[rnk]

I agree, in this case I think the comment will just be extra noise. The comment is likely to outlive the use of these buggy GCC versions. If someone runs into this bug again, it will probably happen somewhere else far away from this code, and the comment isn't going to help them avoid it.

I feel similarly about some MSVC workaround comments. The one that comes to mind is the fact that MSVC always gives unfixed enums the underlying type of int. We shouldn't sprinkle comments about this everywhere, we should document it once well in CodingStandards or something. In this case, I'm not sure we even need that. It seems like something better caught by a static assert.

[rjmccall]

We do have a static assert. I won't insist on the comment.

[mstorsjo]

Ok, so if you're in agreement on this one, can either of you give the stamp of approval? :-)

[rnk]

Pushing code review approval button.

clang/include/clang/AST/Decl.h

protected:
 
   enum DefaultArgKind {
     DAK_None,
     DAK_Unparsed,
     DAK_Uninstantiated,
     DAK_Normal
   };
 
+  enum { NumScopeDepthOrObjCQualsBits = 7 };
+
   class ParmVarDeclBitfields {
     friend class ASTDeclReader;
     friend class ParmVarDecl;
 
     unsigned : NumVarDeclBits;
 
     /// Whether this parameter inherits a default argument from a
     /// prior declaration.
     unsigned HasInheritedDefaultArg : 1;
 
     /// Describes the kind of default argument for this parameter. By default
     /// this is none. If this is normal, then the default argument is stored in
     /// the \c VarDecl initializer expression unless we were unable to parse
     /// (even an invalid) expression for the default argument.
     unsigned DefaultArgKind : 2;
 
     /// Whether this parameter undergoes K&R argument promotion.
     unsigned IsKNRPromoted : 1;
 
     /// Whether this parameter is an ObjC method parameter or not.
     unsigned IsObjCMethodParam : 1;
 
-    enum { NumScopeDepthOrObjCQualsBits = 7 };
-
     /// If IsObjCMethodParam, a Decl::ObjCDeclQualifier.
     /// Otherwise, the number of function parameter scopes enclosing
     /// the function parameter scope in which this parameter was
     /// declared.
     unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
 
     /// The number of parameters preceding this parameter in the
     /// function parameter scope in which it was declared.
public:
   }
 
   unsigned getFunctionScopeDepth() const {
     if (ParmVarDeclBits.IsObjCMethodParam) return 0;
     return ParmVarDeclBits.ScopeDepthOrObjCQuals;
   }
 
   static constexpr unsigned getMaxFunctionScopeDepth() {
-    return (1u << ParmVarDeclBitfields::NumScopeDepthOrObjCQualsBits) - 1;
+    return (1u << NumScopeDepthOrObjCQualsBits) - 1;
   }
 
   /// Returns the index of this parameter in its prototype or method scope.
   unsigned getFunctionScopeIndex() const {
     return getParameterIndex();
   }
 
   ObjCDeclQualifier getObjCDeclQualifier() const {