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

Tyker added a comment. In D63640#2339917 , @rsmith wrote: > In D63640#2331779 , @Tyker wrote: > >> but the "real" blocker is that the testing depends on D85144 >> for testing. >>

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcf34dd0c4e84: [clang] Improve Serialization/Imporing/Dumping of

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

rsmith added a comment. In D63640#2331779 , @Tyker wrote: > but the "real" blocker is that the testing depends on D85144 > for testing. > we could land it marking the tests XFAIL and correct it when dumping >

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

martong added a comment. In D63640#2331779 , @Tyker wrote: > In D63640#2331734 , @martong wrote: > >> In D63640#2331410 , @rsmith wrote: >> >>> Reverse ping: I have a patch

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

Tyker updated this revision to Diff 298318. Tyker added a comment. try to apply martongs's suggestions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/ https://reviews.llvm.org/D63640 Files: clang/include/clang/AST/APValue.h

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

Tyker added a comment. In D63640#2331734 , @martong wrote: > In D63640#2331410 , @rsmith wrote: > >> Reverse ping: I have a patch implementing class type non-type template >> parameters that's blocked on this

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

martong added a comment. In D63640#2331410 , @rsmith wrote: > Reverse ping: I have a patch implementing class type non-type template > parameters that's blocked on this landing. If you won't have time to address > @martong's comments soon, do you mind

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

rsmith added a comment. Reverse ping: I have a patch implementing class type non-type template parameters that's blocked on this landing. If you won't have time to address @martong's comments soon, do you mind if I take this over and land it? Repository: rG LLVM Github Monorepo CHANGES

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

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:9010 + ToPath[Idx] = + cast(const_cast(ImpDecl.get())); +} Tyker wrote: > rsmith wrote: > > We want the path in an `APValue` to be canonical, but importing a canonical >

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

martong requested changes to this revision. martong added a comment. This revision now requires changes to proceed. Sorry for the late review, I just noticed something which is not a logical error, but we could make the ASTImporter code much cleaner. Comment at:

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

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks great, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/ https://reviews.llvm.org/D63640

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

Tyker added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:9010 + ToPath[Idx] = + cast(const_cast(ImpDecl.get())); +} rsmith wrote: > We want the path in an `APValue` to be canonical, but importing a canonical > decl might result

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

Tyker updated this revision to Diff 295626. Tyker marked 15 inline comments as done. Tyker added a comment. addressed comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/ https://reviews.llvm.org/D63640 Files:

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

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

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

Tyker updated this revision to Diff 283985. Tyker added a comment. rebased Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/ https://reviews.llvm.org/D63640 Files: clang/include/clang/AST/APValue.h clang/include/clang/AST/ASTContext.h

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

Tyker added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:11883-11885 ExprResult Result = ActOnFinishFullExpr(Init, VDecl->getLocation(), /*DiscardedValue*/ false, VDecl->isConstexpr()); rsmith wrote: > This may

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

aaron.ballman added inline comments. Comment at: clang/include/clang/AST/APValue.h:537-540 +private: + void setLValueEmptyPath(LValueBase B, const CharUnits , unsigned Size, + bool OnePastTheEnd, bool IsNullPtr); + LValuePathEntry *getLValuePathPtr();

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

Tyker marked 2 inline comments as done. Tyker added inline comments. Comment at: clang/lib/AST/APValue.cpp:176 + (DataSize - sizeof(MemberPointerBase)) / sizeof(CXXRecordDecl *); + typedef CXXRecordDecl *PathElem; union { Tyker wrote: > aaron.ballman

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

Tyker updated this revision to Diff 221169. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/ https://reviews.llvm.org/D63640 Files: clang/include/clang/AST/APValue.h clang/include/clang/AST/ASTContext.h clang/include/clang/AST/ASTImporter.h clang/include/clang/AST/Expr.h

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

Tyker updated this revision to Diff 221166. Tyker marked 24 inline comments as done. Tyker added a comment. Fixed most changes requested by @aaron.ballman test are currently no valid anymore see comments for why. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/

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

Tyker added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:275 - /// Used to cleanups APValues stored in the AST. - mutable llvm::SmallVector APValueCleanups; - aaron.ballman wrote: > Why are you getting rid of this? It seems like we would

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

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

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

martong added a comment. In D63640#1648700 , @Tyker wrote: > Sorry for the long wait. > > Changes: > > - Rebased on current master > - Duplicated test file so that it runs for both importing Thanks for addressing the issues. The ASTImporter related

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

Tyker updated this revision to Diff 217600. Tyker added a comment. Sorry for the long wait. Changes: - Rebased on current master - Duplicated test file so that it runs for both importing CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/ https://reviews.llvm.org/D63640 Files:

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

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:8503 +llvm::Expected ASTImporter::Import(const APValue ) { + APValue Result; Tyker wrote: > martong wrote: > > Tyker wrote: > > > martong wrote: > > > > Looks okay, but could we have

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

Tyker marked 2 inline comments as done. Tyker added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:8503 +llvm::Expected ASTImporter::Import(const APValue ) { + APValue Result; martong wrote: > Tyker wrote: > > martong wrote: > > > Looks okay, but

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

Tyker updated this revision to Diff 208015. Tyker added a comment. fixed the comment for good, sorry for that. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/ https://reviews.llvm.org/D63640 Files: clang/include/clang/AST/APValue.h clang/include/clang/AST/ASTContext.h

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

martong added inline comments. Comment at: clang/include/clang/AST/ASTImporter.h:331 +/// +/// \return the equivalent APValue in the "from" Constext or the import +/// error. martong wrote: > typo: `Constext` -> `Context` > Also, we return with the

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

Tyker added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:8503 +llvm::Expected ASTImporter::Import(const APValue ) { + APValue Result; martong wrote: > Looks okay, but could we have unit tests for this in ASTImporterTest.cpp? I tested importing

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

Tyker updated this revision to Diff 207977. Tyker marked 2 inline comments as done. Tyker set the repository for this revision to rC Clang. Tyker added a comment. fixed comment typo. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/

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

martong added a comment. Comment at: clang/include/clang/AST/ASTImporter.h:331 +/// +/// \return the equivalent APValue in the "from" Constext or the import +/// error. typo: `Constext` -> `Context` Also, we return with the APValue in the "to"

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

Tyker updated this revision to Diff 206467. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/ https://reviews.llvm.org/D63640 Files: clang/include/clang/AST/APValue.h clang/include/clang/AST/ASTContext.h clang/include/clang/AST/ASTImporter.h clang/include/clang/AST/Expr.h

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

Tyker updated this revision to Diff 206464. Tyker edited the summary of this revision. Tyker added a comment. Change: - Add support for LValue, MemberPointer and AddrDiffExpr. - Add tests for importing. i wasn't able to test for some APValueKinds: FixePoint, ComplexInt, ComplexFloat,

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

Tyker updated this revision to Diff 206121. Tyker edited the summary of this revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63640/new/ https://reviews.llvm.org/D63640 Files: clang/include/clang/AST/APValue.h clang/include/clang/AST/ASTContext.h

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

Tyker created this revision. Tyker added a reviewer: rsmith. Herald added a reviewer: martong. Herald added a reviewer: shafik. Herald added a project: clang. Herald added a subscriber: cfe-commits. Tyker edited the summary of this revision. Herald added a subscriber: rnkovacs. Changes: -