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

Mon Sep 23 08:25:45 PDT 2019

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