[PATCH] D133934: [clang][Interp] Handle sizeof() expressions
Timm Bäder via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 16 07:50:56 PDT 2022
tbaeder added inline comments.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:290
+
+ if (!ArgType->isConstantSizeType() ||
+ ArgType->isDependentType())
----------------
erichkeane wrote:
> 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.
Yup, I switched the order around.
================
Comment at: clang/test/AST/Interp/literals.cpp:89
+ static_assert(sizeof(&soint) == sizeof(void*), "");
+ static_assert(sizeof(&soint) == sizeof(nullptr), "");
+
----------------
erichkeane wrote:
> 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?
IIRC it was creating a `Record` for the `RecordDecl` but never freeing it or something similar. The test case didn't do anything anyway since records aren't implemented yet. The only difference now is that I won't notice automatically that a test case suddenly starts working when I implement records.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133934/new/
https://reviews.llvm.org/D133934
More information about the cfe-commits
mailing list