aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/APValue.h:537-540
+private:
+ void setLValueEmptyPath(LValueBase B, const CharUnits &O, unsigned Size,
+ bool OnePastTheEnd, bool IsNullPtr);
+ LValuePathEntry *getLValuePathPtr();
----------------
Rather than add this private bit in the middle of the public interface, you can
move this to the existing private parts.
================
Comment at: clang/include/clang/AST/APValue.h:618
}
+ const CXXRecordDecl **getMemberPointerPathPtr();
};
----------------
We're horribly inconsistent in this class, but because the other private member
functions go with it, this should probably be `GetMemberPointerPathPtr()`.
Maybe rename the get/setLValue methods from above as well?
================
Comment at: clang/include/clang/AST/APValue.h:512
}
- void setVector(const APValue *E, unsigned N) {
+ void ReserveVector(unsigned N) {
assert(isVector() && "Invalid accessor");
----------------
aaron.ballman wrote:
> `reserveVector` per naming conventions
This was marked as done but is still an issue.
================
Comment at: clang/include/clang/AST/ASTContext.h:275
- /// Used to cleanups APValues stored in the AST.
- mutable llvm::SmallVector<APValue *, 0> APValueCleanups;
-
----------------
Tyker wrote:
> 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.
I don't see any new calls to `addDestruction()` though. Have I missed something?
================
Comment at: clang/include/clang/AST/TextNodeDumper.h:149
- const ASTContext *Context;
+ const ASTContext *Context = nullptr;
----------------
Tyker wrote:
> 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.
> there is a big difference is the quality of output between APValue::dump and
> APValue::printPretty.
Yes, there is.
> 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
That would be much-appreciated! When I looked at it, it seemed like it may not
be plausible because `Stmt` does not track which `ASTContext` it is associated
with the same way that `Decl` does, and changing that seemed likely to cause a
huge amount of interface churn.
> 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.
The issue resolved by r372323 was that we would crash on certain kinds of AST
dumps. Specifically, the default AST dumper is often used during a debugging
session to dump AST node information within the debugger. It was trivial to get
that to crash before r372323, but with that revision, we no longer crash but
get slightly uglier output (which is acceptable because it's still
human-readable output).
I'm sorry for causing extra pain for you here, but I didn't want the fix from
this review to accidentally become an enshrined part of the API because it's
very easy to forget about this use case when working on AST dumping
functionality.
================
Comment at: clang/lib/AST/APValue.cpp:748
+APValue::LValuePathEntry *APValue::getLValuePathPtr() {
+ return ((LV *)(char *)Data.buffer)->getPath();
----------------
Tyker wrote:
> 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.
That makes it harder to call this helper from a constant context. I think there
should be overloads (one `const`, one not) to handle this.
================
Comment at: clang/test/ASTMerge/APValue/APValue.cpp:1
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
----------------
Can remove the spurious newline. Also, it seems this file was added with svn
properties, was that intentional (we don't usually do that, FWIW)?
================
Comment at: clang/test/ASTMerge/APValue/APValue.cpp:2-3
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -ast-merge %t.pch -ast-dump-all |
FileCheck %s
+
----------------
no need for `-x c++` is there? This is already a C++ compilation unit.
================
Comment at: clang/test/ASTMerge/APValue/APValue.cpp:28
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
----------------
Are you planning to address this in this patch? Also, I think it's FixedPoint
and not FixePoint.
================
Comment at: clang/test/PCH/APValue.cpp:1
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
----------------
Spurious newline.
================
Comment at: clang/test/PCH/APValue.cpp:2
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -include-pch %t.pch -ast-dump-all |
FileCheck %s
----------------
`-x c++` ?
It's really unfortunate that these test files are identical copies in different
directories.
================
Comment at: clang/test/PCH/APValue.cpp:28
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
----------------
Same comment here.
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
