aaron.ballman 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}}
+
----------------
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).
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
