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