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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits