[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues
Tyker via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Sep 21 04:35:09 PDT 2019
Tyker added inline comments.
================
Comment at: clang/include/clang/AST/ASTContext.h:275
- /// Used to cleanups APValues stored in the AST.
- mutable llvm::SmallVector<APValue *, 0> APValueCleanups;
-
----------------
aaron.ballman wrote:
> Why are you getting rid of this? It seems like we would still want these cleaned up.
when i added APValueCleanups i wasn't aware that there were a generic system to handle this. but with this patch APValue a cleaned up using the generic ASTContext::addDestruction.
================
Comment at: clang/include/clang/AST/TextNodeDumper.h:149
- const ASTContext *Context;
+ const ASTContext *Context = nullptr;
----------------
aaron.ballman wrote:
> Good catch -- this pointed out a bug in the class that I've fixed in r372323, so you'll need to rebase.
i took a look at the revision. there is a big difference is the quality of output between APValue::dump and APValue::printPretty. i think it is possible to come quite close to printPretty's output even without the ASTContext. this would require having a default PrintingPolicy and improving dump
in this patch i was relying on the -ast-dump output for testing. i would need to find an other testing strategy or make the improvement to APValue::dump first.
================
Comment at: clang/lib/AST/APValue.cpp:176
+ (DataSize - sizeof(MemberPointerBase)) / sizeof(CXXRecordDecl *);
+ typedef CXXRecordDecl *PathElem;
union {
----------------
aaron.ballman wrote:
> Why is this no longer a pointer to `const`?
when imporing or deserializing, we reserve the space for elements and then import/deserialize element directly in place. so the buffer storing them is not const. that said i saw that the normal construction cast away the const.
================
Comment at: clang/lib/AST/APValue.cpp:748
+APValue::LValuePathEntry *APValue::getLValuePathPtr() {
+ return ((LV *)(char *)Data.buffer)->getPath();
----------------
aaron.ballman wrote:
> Can this function be marked `const`?
this function gives access to non-const internal data. this function is private so the impact is quite limited.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63640/new/
https://reviews.llvm.org/D63640
More information about the cfe-commits
mailing list