ZarkoCA added inline comments.
================
Comment at: clang/test/Sema/aix-attr-align.c:34-35
+ baz(s.a); // no-warning
+ baz(s.b); // expected-warning {{requesting an alignment of 16 bytes or
greater for struct members is not binary compatible with IBM XL C/C++ for AIX
16.1.0 and older}}
+ baz(s.c); // expected-warning {{requesting an alignment of 16 bytes or
greater for struct members is not binary compatible with IBM XL C/C++ for AIX
16.1.0 and older}}
+
----------------
aaron.ballman wrote:
> ZarkoCA wrote:
> > aaron.ballman wrote:
> > > This diagnostic is a bit odd to me. It says there's a request for
> > > alignment, but there's no such request on this line. So it's not clear
> > > how the user is supposed to react to the diagnostic. While the current
> > > code makes it somewhat obvious because there's only one field in the
> > > expression, imagine code like `quux(s.a, s.b);` where it's less clear as
> > > to which field causes the diagnostic from looking at the call site.
> > >
> > > Personally, I found the old diagnostics to be more clear as to what the
> > > issue is. I think we should put the warning on the declaration involving
> > > the alignment request, and we should add a note to the call site where
> > > the diagnostic is generated from (or vice versa). WDYT?
> > That's a good point actually, there's nothing on the line that would be
> > obvious to a user.
> >
> > I opted to warn at the use of struct member and to make a note where it was
> > declared. This will hopefully help with determining which struct member is
> > causing this warning instead of searching the code for its cause. I have a
> > slight preference for doing it this way instead of the other way but can
> > change it if preferred.
> I'd like to understand why you have a preference for this way around.
>
> The use is the point in time at which we know there's a problem, so I
> definitely agree with waiting until the struct member is used to diagnose
> anything.
>
> But, to my thinking, the use is not the cause of the issue; the declaration
> is what's problematic. With that perspective, it seems like we want the
> warning and the note the other way around: warn about the structure member
> declaration being the issue, and note where the use that triggered the
> complaint about the declaration. Then the warning diagnostic is associated
> most closely with the code that needs to be adjusted by the user in order to
> silence the warning. This also makes it easier for the user to silence the
> warning with pragmas (you can put the suppression mechanism in one place, at
> the declaration site, instead of sprinkling it all over at the use sites).
> Then the warning diagnostic is associated most closely with the code that
> needs to be adjusted by the user in order to silence the warning.
Thanks, that's a good point and your additional argument about silencing it
with pragmas at a single place has convinced me to switch it up.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118350/new/
https://reviews.llvm.org/D118350
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
