erichkeane added inline comments.
================ Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:290 + + if (!ArgType->isConstantSizeType() || + ArgType->isDependentType()) ---------------- tbaeder wrote: > erichkeane wrote: > > Documentation for `isConstantSizedType` says it isn't legal to call it on > > dependent or incomplete types. so this probably needs to be: > > > > ``` if (ArgType->isDependentType() || ArgType->isIncompleteType() || > > !ArgType->isConstantSizedType())``` > > > > In roughly that order. > `isDependentType()` is implicitly assumed, isn't it? `ExprConstant.cpp` > asserts everywhere that neither types nor expressions are (value)dependent. > It also doesn't handle that in `HandleSizeof`. Ok, good to know about that then. I saw you'd tested for isDependentType above, but isConstantSizedType would assert in that case. ================ Comment at: clang/test/AST/Interp/literals.cpp:89 + static_assert(sizeof(&soint) == sizeof(void*), ""); + static_assert(sizeof(&soint) == sizeof(nullptr), ""); + ---------------- tbaeder wrote: > aaron.ballman wrote: > > This test got me thinking about another test case: > > ``` > > struct S { > > void func(); > > }; > > > > constexpr void (S::*Func)() = &S::func; > > static_assert(sizeof(Func) == sizeof(&S::func), ""); > > ``` > > (Can't test this one against `nullptr` because `sizeof(nullptr)` is not > > telling you the size of a pointer to member function despite `nullptr` > > being convertible to one.) > I'll add it, but might have to `#if 0` it out for the same reason I had to > push https://reviews.llvm.org/rGa8843643cd75d0e93ebcf3f30b470d2b8e59868d What is the cause of that leak? Is that something we should be fixing ASAP so we can enable these tests? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133934/new/ https://reviews.llvm.org/D133934 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits