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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/APValue.h:613
+  /// in place as after importing/deserializating then.
+  void reserveVector(unsigned N) {
+    assert(isVector() && "Invalid accessor");
----------------
`ReserveVector`


================
Comment at: clang/include/clang/AST/APValue.h:620
+                              unsigned Size);
+  void setLValueEmptyPath(LValueBase B, const CharUnits &O, unsigned Size,
+                          bool OnePastTheEnd, bool IsNullPtr);
----------------
`SetLValueEmptyPath`


================
Comment at: clang/lib/AST/APValue.cpp:599
         Out << '[' << Path[I].getAsArrayIndex() << ']';
-        ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+        ElemTy = cast<ArrayType>(ElemTy)->getElementType();
       }
----------------
Tyker wrote:
> aaron.ballman wrote:
> > Are you sure this doesn't change behavior? See the implementation of `ASTContext::getAsArrayType()`. Same question applies below.
> i ran the test suite after the change it there wasn't any test failures. but the test on dumping APValue are probably not as thorough as we would like them to be.
> from analysis of `ASTContext::getAsArrayType()` the only effect i see on the element type is de-sugaring and canonicalization which shouldn't affect correctness of the output.  de-sugaring requires the ASTContext but canonicalization doesn't.
> 
> i think the best way the have higher confidence is to ask rsmith what he thinks.
Yeah, I doubt we have good test coverage for all the various behaviors here. I was wondering if the qualifiers bit was handled properly with a simple cast. @rsmith is a good person to weigh in.


================
Comment at: clang/test/ASTMerge/APValue/APValue.cpp:28
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
----------------
Tyker wrote:
> aaron.ballman wrote:
> > 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.
> the reason i proposed `llvm_unreachable` was because it passes the tests and prevents future developer from depending on the code that depend on it assuming it works.
We typically only use `llvm_unreachable` for situations where we believe the code path is impossible to reach, which is why I think that's the wrong approach. We could use an assertion to test the theory, however.


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

https://reviews.llvm.org/D63640