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