aaron.ballman 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
+ {
----------------
inclyc wrote:
> 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?
> 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
Yeah, that is interesting!
> Emm, so, how about switch back to RAIIObject to support diagnosing nested
> definitions?
I'm getting more comfortable with that approach. Please be sure to add C++
tests to make sure we diagnose in C and C++ the same way in terms of nested
structures. Thank you for exploring the approaches with me!
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
