[PATCH] D133934: [clang][Interp] Handle sizeof() expressions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D133934#3794389 <https://reviews.llvm.org/D133934#3794389>, @tbaeder wrote:

> I can see that `HandleSizeOf()` uses 1 for void and function types as a gcc extension, but I can't reproduce that: https://godbolt.org/z/njG9zh6PM

C code allows the construct: https://godbolt.org/z/5KfY9PPqa

Worth keeping in mind: C2x has support for `constexpr` objects, so we are going to have to care about this. We've not started doing the work to enable `constexpr` for C yet, so there's some wiggle room in terms of *when* we have to care, but we should try to keep C in mind when working on the new interpreter as much as is reasonable.



================
Comment at: clang/lib/AST/Interp/Boolean.h:50
 
   explicit operator unsigned() const { return V; }
+  explicit operator int8_t() const { return V; }
----------------
At some point, do we want to rename this (and int) to use uint32_t/int32_t?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:290
+    return this->emitConst(
+        E, Ctx.getASTContext().getTypeSizeInChars(ArgType).getQuantity());
+  }
----------------
tbaeder wrote:
> shafik wrote:
> > aaron.ballman wrote:
> > > erichkeane wrote:
> > > > shafik wrote:
> > > > > I notice that `HandleSizeof` special cases `void` and function types. 
> > > > OOOH, DEFINITELY make sure you test function types here, and figure out how HandleSizeof does it.
> > > > 
> > > > BUT, I think 'void' should error/be an invalid before we get here?
> > > > 
> > > > ALSO, now that I think further, I'm sure @aaron.ballman has a bunch of runtime-evaluated sizeof examples to share around vlas.
> > > I was just about to comment about everyone's favorite horrible C++ extension. :-D We should make sure we properly reject code like:
> > > ```
> > > void func() {
> > >   int n = 12;
> > >   constexpr int oofda = sizeof(int[n++]);
> > > }
> > > ```
> > > while accepting code like:
> > > ```
> > > consteval int foo(int n) {
> > >   return sizeof(int[n]);
> > > }
> > > constinit int var = foo(5);
> > > ```
> > Note, that clang currently treats that as ill-formed and there is divergence with how gcc and clang treat the `constexpr` case as well.
> > 
> > So if we are going to say we want this to work then we should file a bug against clang. 
> Right, the second example doesn't seem to be accepted by current clang.
I'd add both of the test cases, put a FIXME comment above the one that currently fails but we'd like to see succeed. We can work on fixing that fixme whenever we get around to it.


================
Comment at: clang/test/AST/Interp/literals.cpp:89
+  static_assert(sizeof(&soint) == sizeof(void*), "");
+  static_assert(sizeof(&soint) == sizeof(nullptr), "");
+
----------------
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.)


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

https://reviews.llvm.org/D133934