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

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.


================
Comment at: clang/lib/AST/APValue.cpp:599
         Out << '[' << Path[I].getAsArrayIndex() << ']';
-        ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+        ElemTy = cast<ArrayType>(ElemTy)->getElementType();
       }
----------------
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.


================
Comment at: clang/test/ASTMerge/APValue/APValue.cpp:28
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
----------------
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.


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

https://reviews.llvm.org/D63640