inclyc added inline comments.

================
Comment at: clang/test/Sema/offsetof.c:79
+    int a;
+    struct B // no-error, struct B is not defined within __builtin_offsetof 
directly
+    {
----------------
aaron.ballman wrote:
> inclyc wrote:
> > inclyc wrote:
> > > aaron.ballman wrote:
> > > > inclyc wrote:
> > > > > aaron.ballman wrote:
> > > > > > I think this is defensible. The wording in the standard is "If the 
> > > > > > specified type defines a new type or if the specified member is a 
> > > > > > bit-field, the behavior is undefined." and the specified type in 
> > > > > > this case is `struct A`; that `struct A` happens to also define 
> > > > > > `struct B` is immaterial.
> > > > > > 
> > > > > > However, the intent behind the change to the rule is to support 
> > > > > > older implementations of `offsetof` to protect them from having to 
> > > > > > deal with a case like: `offsetof(struct S { int a, b }, b);` where 
> > > > > > `offsetof` is a macro and thus the comma between `a` and `b` is 
> > > > > > treated as a separator. So there's a part of me that wonders if we 
> > > > > > want to also support diagnosing this case. But then we'd have to 
> > > > > > look at the declarator context more recursively to see whether any 
> > > > > > of the contexts on the stack are an `offsetof` context and that 
> > > > > > might be tricky.
> > > > > > 
> > > > > > Thoughts?
> > > > > FWIW, gcc seems just rejects all definitions in this context. 
> > > > > (Perhaps during Parsing the statements). If we add a bool state to 
> > > > > the Parser (just using RAII object as before) struct B will trigger 
> > > > > diagnostic error because the state "ParsingOffsetof" is passed into 
> > > > > inner declaration.
> > > > GCC accepts currently: https://godbolt.org/z/oEvzjW6Ee but you're 
> > > > correct regarding switching back to an RAII object being an easier way 
> > > > to address the nested declarations.
> > > > 
> > > > Let me think on this situation a bit....
> > > > GCC accepts currently
> > > 
> > > C++: https://godbolt.org/z/fon8e7dzf 
> > ```
> > <source>: In function 'int main()':
> > <source>:3:3: error: types may not be defined within '__builtin_offsetof'
> >     3 |   {
> >       |   ^
> > <source>:6:5: error: types may not be defined within '__builtin_offsetof'
> >     6 |     {
> >       |     ^
> > Compiler returned: 1
> > ```
> C++ is a different language in this case though. In C, you can generally 
> define types anywhere you can spell a type, and in C++ you cannot. e.g., 
> `void func(struct S { int x, y; } s);` is valid in C and invalid in C++.
GCC explicitly reject type definitions since GCC 8 (in C++ mode). I guess the 
logic here in GCC may also add a boolean state parameter to parser.

Interestingly, in C++ we treat the first parameter of `__builtin_offsetof` as a 
type specifier, rejecting the definition of `struct A`, but not rejecting 
nested definition `struct B`

https://godbolt.org/z/PqWjzqqYn

Emm, so, how about switch back to RAIIObject to support diagnosing nested 
definitions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to