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

[Tyker via Phabricator via cfe-commits]

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