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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/AST/APValue.h:651-653
+  /// The following functions are used as part of initialization. during
+  /// Deserialization and Importing. Reserve the space then write element
+  /// directly in place as after importing/deserialization then.
----------------
Maybe something like this?


================
Comment at: clang/include/clang/AST/APValue.h:613
+  /// in place as after importing/deserializating then.
+  void reserveVector(unsigned N) {
+    assert(isVector() && "Invalid accessor");
----------------
aaron.ballman wrote:
> `ReserveVector`
This function changes the size of the vector, so `Reserve` doesn't seem right to me. How about `setVectorUninit`? (Generally including `Uninit` in the names of the setters that leave things uninitialized would be useful.)

Also, instead of exposing `GetInternal` functions, how about adding functions that return a `MutableArrayRef` holding the array:

```
MutableArrayRef<APValue> setVectorUninit(unsigned N);
MutableArrayRef<LValuePathEntry> setLValueUninit(LValueBase B, const CharUnits &O, unsigned Size, bool OnePastTheEnd, bool IsNullPtr);
MutableArrayRef<const CXXRecordDecl*> MakeMemberPointerUninit(const ValueDecl *Member, bool IsDerivedMember, unsigned Size);
```

(It would be nice if `MakeMemberPointer` weren't so inconsistent with everything else this class does. But this whole interface is a mess.)


================
Comment at: clang/lib/AST/APValue.cpp:779
 
-void APValue::MakeMemberPointer(const ValueDecl *Member, bool IsDerivedMember,
-                                ArrayRef<const CXXRecordDecl*> Path) {
+void APValue::MakeEmptyMemberPointer(const ValueDecl *Member,
+                                     bool IsDerivedMember, unsigned Size) {
----------------
Similarly maybe mentioning `Uninit` here would be useful.


================
Comment at: clang/lib/AST/ASTImporter.cpp:6680-6687
 
-  APValue::ValueKind Kind = E->getResultAPValueKind();
-  if (Kind == APValue::Int || Kind == APValue::Float ||
-      Kind == APValue::FixedPoint || Kind == APValue::ComplexFloat ||
-      Kind == APValue::ComplexInt)
-    return ConstantExpr::Create(Importer.getToContext(), ToSubExpr,
-                                E->getAPValueResult());
-  return ConstantExpr::Create(Importer.getToContext(), ToSubExpr);
+  // APValue::ValueKind Kind = E->getResultAPValueKind();
+  // if (Kind == APValue::Int || Kind == APValue::Float ||
+  //     Kind == APValue::FixedPoint || Kind == APValue::ComplexFloat ||
+  //     Kind == APValue::ComplexInt)
+  //   return ConstantExpr::Create(Importer.getToContext(), ToSubExpr,
+  //                               E->getAPValueResult());
----------------
Delete this commented-out code, please.


================
Comment at: clang/lib/AST/ASTImporter.cpp:9010
+      ToPath[Idx] =
+          cast<const CXXRecordDecl>(const_cast<Decl *>(ImpDecl.get()));
+    }
----------------
We want the path in an `APValue` to be canonical, but importing a canonical decl might result in a non-canonical decl.


================
Comment at: clang/lib/AST/ASTImporter.cpp:9030-9031
+        } else {
+          FromElemTy =
+              FromValue.getLValueBase().get<const ValueDecl *>()->getType();
+          llvm::Expected<const Decl *> ImpDecl =
----------------
If you're intentionally not handling `DynamicAllocLValue` here (because those should always be transient), a comment to that effect would be useful.


================
Comment at: clang/lib/AST/ASTImporter.cpp:9071
+      for (unsigned LoopIdx = 0; LoopIdx < PathLength; LoopIdx++) {
+        if (FromElemTy->getAs<RecordType>()) {
+          const Decl *FromDecl =
----------------



================
Comment at: clang/lib/Serialization/ASTReader.cpp:8973-8978
+    const llvm::fltSemantics &FloatSema1 = llvm::APFloatBase::EnumToSemantics(
+        static_cast<llvm::APFloatBase::Semantics>(asImpl().readUInt32()));
     llvm::APFloat First = readAPFloat(FloatSema1);
-    const llvm::fltSemantics &FloatSema2 = readAPFloatSemantics(*this);
-    return APValue(std::move(First), readAPFloat(FloatSema2));
-  }
-  case APValue::LValue:
-  case APValue::Vector:
-  case APValue::Array:
-  case APValue::Struct:
-  case APValue::Union:
-  case APValue::MemberPointer:
-  case APValue::AddrLabelDiff:
-    // TODO : Handle all these APValue::ValueKind.
-    return APValue();
+    const llvm::fltSemantics &FloatSema2 = llvm::APFloatBase::EnumToSemantics(
+        static_cast<llvm::APFloatBase::Semantics>(asImpl().readUInt32()));
+    return APValue(std::move(First), asImpl().readAPFloat(FloatSema2));
----------------
You can assume here (and assert in the writer) that both floats in a `ComplexFloat` have the same `fltSemantics`.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9027
+    for (unsigned LoopIdx = 0; LoopIdx < PathSize; LoopIdx++)
+      PathArray[LoopIdx] = asImpl().readDeclAs<const CXXRecordDecl>();
+    return Result;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63640