[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/APValue.h:512
   }
-  void setVector(const APValue *E, unsigned N) {
+  void ReserveVector(unsigned N) {
     assert(isVector() && "Invalid accessor");
----------------
`reserveVector` per naming conventions


================
Comment at: clang/include/clang/AST/ASTContext.h:275
-  /// Used to cleanups APValues stored in the AST.
-  mutable llvm::SmallVector<APValue *, 0> APValueCleanups;
-
----------------
Why are you getting rid of this? It seems like we would still want these cleaned up.


================
Comment at: clang/include/clang/AST/Expr.h:957
   /// Describes the kind of result that can be trail-allocated.
+  /// Enumerator need to stay ordered by size of there storage.
   enum ResultStorageKind { RSK_None, RSK_Int64, RSK_APValue };
----------------
Enumerator -> Enumerators
there -> their


================
Comment at: clang/include/clang/AST/TextNodeDumper.h:149
 
-  const ASTContext *Context;
+  const ASTContext *Context = nullptr;
 
----------------
Good catch -- this pointed out a bug in the class that I've fixed in r372323, so you'll need to rebase.


================
Comment at: clang/lib/AST/APValue.cpp:176
+      (DataSize - sizeof(MemberPointerBase)) / sizeof(CXXRecordDecl *);
+  typedef CXXRecordDecl *PathElem;
   union {
----------------
Why is this no longer a pointer to `const`?


================
Comment at: clang/lib/AST/APValue.cpp:748
 
+APValue::LValuePathEntry *APValue::getLValuePathPtr() {
+  return ((LV *)(char *)Data.buffer)->getPath();
----------------
Can this function be marked `const`?


================
Comment at: clang/lib/AST/ASTImporter.cpp:8745
 
+<<<<<<< HEAD
 Expected<DeclarationName> ASTImporter::HandleNameConflict(DeclarationName Name,
----------------
Conflict markers?


================
Comment at: clang/lib/AST/ASTImporter.cpp:8808
+    Result.MakeUnion();
+    auto ImpFDecl = Import(FromValue.getUnionField());
+    if (!ImpFDecl) {
----------------
Please don't use auto as the type is not spelled out in the initialization.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8813
+    }
+    auto ImpValue = Import(FromValue.getUnionValue());
+    if (!ImpValue) {
----------------
Please don't use `auto` as the type is not spelled out in the initialization.

(Same elsewhere, I'll stop commenting on them.)


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:691-695
+    if (Context)
+      Node->getAPValueResult().printPretty(OS, *Context, Node->getType());
+    else
+      Node->getAPValueResult().dump(OS);
+  } else {}
----------------
You'll have to rebase this too -- these changes shouldn't be needed any longer.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11315
 
+  if (VDecl->isConstexpr()) {
+    Init = ConstantExpr::Create(
----------------
Can elide braces.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9601
+  case APValue::Union: {
+    FieldDecl *FDecl =
+        cast<FieldDecl>(GetDecl(ReadDeclID(F, Record, RecordIdx)));
----------------
`auto *`


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9607-9608
+  case APValue::AddrLabelDiff: {
+    AddrLabelExpr *LHS = cast<AddrLabelExpr>(ReadExpr(F));
+    AddrLabelExpr *RHS = cast<AddrLabelExpr>(ReadExpr(F));
+    return APValue(LHS, RHS);
----------------
`auto *`


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9614
+    bool IsDerived = Record[RecordIdx++];
+    ValueDecl *Member =
+        cast<ValueDecl>(GetDecl(ReadDeclID(F, Record, RecordIdx)));
----------------
`auto *`


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9625-9631
+    uint64_t tmp = Record[RecordIdx++];
+    bool hasLValuePath = tmp & 0x1;
+    bool isLValueOnePastTheEnd = tmp & 0x2;
+    bool isExpr = tmp & 0x4;
+    bool isTypeInfo = tmp & 0x8;
+    bool isNullPtr = tmp & 0x10;
+    bool hasBase = tmp & 0x20;
----------------
These names don't match the current coding conventions.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9634
+    QualType ElemTy;
+    assert(!isExpr || !isTypeInfo && "LValueBase cannot be both");
+    if (hasBase) {
----------------
You should add parens here to avoid diagnostics.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:5485
+  case APValue::LValue: {
+    uint64_t tmp = Value.hasLValuePath() | Value.isLValueOnePastTheEnd() << 1 |
+                   Value.getLValueBase().is<const Expr *>() << 2 |
----------------
Name doesn't match coding conventions.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:5496
+        push_back(Value.getLValueBase().getVersion());
+        if (auto *E = Value.getLValueBase().dyn_cast<const Expr *>()) {
+          AddStmt(const_cast<Expr *>(E));
----------------
`const auto *`


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:5518
+          const Decl *BaseOrMember = Elem.getAsBaseOrMember().getPointer();
+          if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(BaseOrMember)) {
+            AddDeclRef(RD);
----------------
`const auto *`


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:5522
+          } else {
+            const ValueDecl *VD = cast<ValueDecl>(BaseOrMember);
+            AddDeclRef(VD);
----------------
`const auto *`


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:5538
 
-void ASTWriter::AddIdentifierRef(const IdentifierInfo *II, RecordDataImpl &Record) {
+void ASTWriter::AddIdentifierRef(const IdentifierInfo *II,
+                                 RecordDataImpl &Record) {
----------------
Unrelated change?


================
Comment at: clang/lib/Serialization/ASTWriterStmt.cpp:42
 
-    ASTStmtWriter(const ASTStmtWriter&) = delete;
+  ASTStmtWriter(const ASTStmtWriter &) = delete;
 
----------------
It seems like there's a lot of unrelated formatting changes in this file that should not be part of the patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640