[PATCH] D118350: [Clang][Sema][AIX][PowerPC] Emit byval alignment warning only when struct member is passed to a function

[Zarko Todorovski via Phabricator via cfe-commits]

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