[PATCH] D49729: [AST][1/4] Move the bit-fields from TagDecl, EnumDecl and RecordDecl into DeclContext

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

This seems like a patch with a good direction, I generally think that this change doesn't change the readability of the code (and generally matches the structure of other code in clang).  It is generally a mechanical change, though I'd suggest running clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting) along this patch.



================
Comment at: include/clang/AST/Decl.h:3115
+  /// True if this decl has its body fully specified.
+  void setCompleteDefinition(bool V = true) {
+    TagDeclBits.IsCompleteDefinition = V;
----------------
The setters that you've moved change the accessibility of these variables.  I'd much prefer we maintain the protected/privateness of these.  In a few of these cases, they shouldn't be modified outside of the Decl itself


================
Comment at: include/clang/AST/Decl.h:3155
+
+  bool mayHaveOutOfDateDef() const {
+    return TagDeclBits.MayHaveOutOfDateDef;
----------------
These two should also be documented.


================
Comment at: include/clang/AST/Decl.h:3586
+    return RecordDeclBits.HasFlexibleArrayMember;
+  }
+  void setHasFlexibleArrayMember(bool V) {
----------------
While you're reflowing this, put a new line between teh definitions throughout the patch (of ones where you're modifying them).


================
Comment at: include/clang/AST/DeclBase.h:1253
 ///   TranslationUnitDecl
+///   ExternCContext
 ///   NamespaceDecl
----------------
While this documentation change is appreciated, I believe it would fit better in a separate patch.


================
Comment at: include/clang/AST/DeclBase.h:1312
+
+  /// Stores the bits used by TagDecl.
+  /// If modified NumTagDeclBits and the accessor
----------------
The newlines in this comment feel strange, did you run clang-format on these?


================
Comment at: include/clang/AST/DeclBase.h:1603
+
+    // Not a bitfield but this save space.
+    // Note that ObjCContainerDeclBitfields is full.
----------------
s/save/saves


================
Comment at: include/clang/AST/DeclBase.h:2299
+
+  /// Whether this declaration context has had external visible
+  /// storage added since the last lookup. In this case, \c LookupPtr's
----------------
'externally visible'?


================
Comment at: include/clang/AST/DeclBase.h:2345
 private:
-  friend class DependentDiagnostic;
+  /// State that this declaration context has had external visible
+  /// storage added since the last lookup. In this case, \c LookupPtr's
----------------
Again, externally visible?


================
Comment at: lib/AST/DeclBase.cpp:991
+  setUseQualifiedLookup(false);
+  static_assert(sizeof(DeclContextBitfields) <= 8,
+      "DeclContextBitfields is larger than 8 bytes!");
----------------
Is there value to having these static-asserts in the constructor here?  It seems that it would go best under the 'union' that includes all of these.


Repository:
  rC Clang

https://reviews.llvm.org/D49729