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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
