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