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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/APValue.h:618
   }
+  const CXXRecordDecl **getMemberPointerPathPtr();
 };
----------------
Tyker wrote:
> 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.
Agreed -- I wasn't suggesting to fix the whole class, but just the new APIs that we add to the class. It looks like the private functions most consistently use a capital letter in this class, unfortunately. Best to stick with the local convention when in conflict.


================
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:
> > 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.
> 
Ahhh, thank you for the explanation, I was missing that context.


================
Comment at: clang/include/clang/AST/PrettyPrinter.h:203
 
+  /// Wether null pointers should be printed as nullptr or as NULL.
+  unsigned UseNullptr : 1;
----------------
Wether -> Whether


================
Comment at: clang/lib/AST/APValue.cpp:748
 
+APValue::LValuePathEntry *APValue::getLValuePathPtr() {
+  return ((LV *)(char *)Data.buffer)->getPath();
----------------
Tyker wrote:
> 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`
Nothing about this API suggests that. The name looks like a generic getter. Perhaps a more descriptive name and some comments would help?


================
Comment at: clang/lib/AST/APValue.cpp:537
 
-      if (const ValueDecl *VD = Base.dyn_cast<const ValueDecl*>())
+      if (const ValueDecl *VD = Base.dyn_cast<const ValueDecl *>())
         Out << *VD;
----------------
Since you're touching the code anyway, this can be `const auto *`.


================
Comment at: clang/lib/AST/APValue.cpp:599
         Out << '[' << Path[I].getAsArrayIndex() << ']';
-        ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+        ElemTy = cast<ArrayType>(ElemTy)->getElementType();
       }
----------------
Are you sure this doesn't change behavior? See the implementation of `ASTContext::getAsArrayType()`. Same question applies below.


================
Comment at: clang/lib/AST/APValue.cpp:614
   case APValue::Array: {
-    const ArrayType *AT = Ctx.getAsArrayType(Ty);
+    const ArrayType *AT = cast<ArrayType>(Ty);
     QualType ElemTy = AT->getElementType();
----------------
`const auto *` and same question about behavior changes.


================
Comment at: clang/test/ASTMerge/APValue/APValue.cpp:1
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
----------------
Tyker wrote:
> 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 ?
No idea; I'm on a platform where file modes are ignored. You should probably drop the svn property.


================
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
+
----------------
Tyker wrote:
> 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'`
There's no `%s` on that line for the source file, which is why you get that diagnostic. I'm not certain what that RUN line does, actually -- it does an AST merge with... nothing... and then prints it out?

If that's intended, then you only need the `-x c++` on that one RUN line.


================
Comment at: clang/test/ASTMerge/APValue/APValue.cpp:28
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
----------------
Tyker wrote:
> 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 ?
I don't think `llvm_unreachable` makes a whole lot of sense unless the code is truly unreachable because there's no way to get an AST with that representation. By code inspection, the code looks reasonable but it does make me a bit uncomfortable to adopt it without tests. I suppose the FIXME is a reasonable compromise in this case, but if you have some spare cycles to investigate ways to get those representations, it would be appreciated.


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

https://reviews.llvm.org/D63640