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

Mon Sep 23 12:32:34 PDT 2019

Tyker added inline comments.

================
Comment at: clang/include/clang/AST/APValue.h:618
   }
+  const CXXRecordDecl **getMemberPointerPathPtr();
 };
----------------
aaron.ballman wrote:
> 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?
> We're horribly inconsistent in this class

this class has many flaws. but is far too broadly used to fix.

================
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:
> aaron.ballman wrote:
> > `reserveVector` per naming conventions
> This was marked as done but is still an issue.
sorry

================
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:
> 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?
the modification to use `addDestruction()` was made in a previous revision (https://reviews.llvm.org/D63376).
the use is currently on master in `ConstantExpr::MoveIntoResult` in the RSK_APValue case of the switch.
this is just a removing an unused member.

================
Comment at: clang/include/clang/AST/TextNodeDumper.h:149

-  const ASTContext *Context;
+  const ASTContext *Context = nullptr;

----------------
aaron.ballman wrote:
> 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.
no worries, i wrote the original bug. i added APValue::dumpPretty which has almost the same output as APValue::printPretty but doesn't need an ASTContext. and is used for TextNodeDumper.

================
Comment at: clang/lib/AST/APValue.cpp:748

+APValue::LValuePathEntry *APValue::getLValuePathPtr() {
+  return ((LV *)(char *)Data.buffer)->getPath();
----------------
aaron.ballman wrote:
> 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.
this helper is not intended to be used outside of importing and serialization. it is logically part of initialization.
normal users are intended to use `ArrayRef<APValue::LValuePathEntry> APValue::getLValuePath() const`

================
Comment at: clang/test/ASTMerge/APValue/APValue.cpp:1
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
----------------
aaron.ballman wrote:
> 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)?
it wasn't intentional, i added via `git add` i don't think i did anything weird. is it a problem ?

================
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
+
----------------
aaron.ballman wrote:
> no need for `-x c++` is there? This is already a C++ compilation unit.
i don't know if it is normal. but i am getting an error hen i am not using `-x c++`
`error: invalid argument '-std=gnu++2a' not allowed with 'C'`

================
Comment at: clang/test/ASTMerge/APValue/APValue.cpp:28
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
----------------
aaron.ballman wrote:
> Are you planning to address this in this patch? Also, I think it's FixedPoint and not FixePoint.
i don't intend to add them in this patch or subsequent patches. i don't know how to use the features that have these representations, i don't even know if they can be stored stored in that AST. so this is untested code.
that said theses representations aren't complex. the imporing for FixePoint, ComplexInt, ComplexFloat is a no-op and for AddrLabelDiff it is trivial. for serialization, I can put an llvm_unreachable to mark them as untested if you want ?

================
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
----------------
aaron.ballman wrote:
> `-x c++` ?
> 
> It's really unfortunate that these test files are identical copies in different directories.
they are almost the same. the second run line is slightly different. i had a version with both run line in the same file. but martong was worried that people running partial test would only not get all test relevant for what they were testing.

after through. ASTMerge requires both serialization and importing so it would probably be sufficient ?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640