Thanks, I recommitted it in r309386. On 27 July 2017 at 15:53, Hans Wennborg <h...@chromium.org> wrote:
> On Thu, Jul 27, 2017 at 3:41 AM, Alex L <arpha...@gmail.com> wrote: > > > > > > On 26 July 2017 at 22:32, Hans Wennborg <h...@chromium.org> wrote: > >> > >> On Wed, Jul 26, 2017 at 11:27 AM, Hans Wennborg <h...@chromium.org> > wrote: > >> > On Wed, Jul 26, 2017 at 5:20 AM, Alex Lorenz via cfe-commits > >> > <cfe-commits@lists.llvm.org> wrote: > >> >> Author: arphaman > >> >> Date: Wed Jul 26 05:20:57 2017 > >> >> New Revision: 309106 > >> >> > >> >> URL: http://llvm.org/viewvc/llvm-project?rev=309106&view=rev > >> >> Log: > >> >> Recommit r308327 2nd time: Add a warning for missing > >> >> '#pragma pack (pop)' and suspicious uses of '#pragma pack' in > included > >> >> files > >> >> > >> >> The first recommit (r308441) caused a "non-default #pragma pack value > >> >> might > >> >> change the alignment of struct or union members in the included file" > >> >> warning > >> >> in LLVM itself. This recommit tweaks the added warning to avoid > >> >> warnings for > >> >> #includes that don't have any records that are affected by the > >> >> non-default > >> >> alignment. This tweak avoids the previously emitted warning in LLVM. > >> >> > >> >> Original message: > >> >> > >> >> This commit adds a new -Wpragma-pack warning. It warns in the > following > >> >> cases: > >> >> > >> >> - When a translation unit is missing terminating #pragma pack (pop) > >> >> directives. > >> >> - When entering an included file if the current alignment value as > >> >> determined > >> >> by '#pragma pack' directives is different from the default > alignment > >> >> value. > >> >> - When leaving an included file that changed the state of the current > >> >> alignment > >> >> value. > >> >> > >> >> rdar://10184173 > >> >> > >> >> Differential Revision: https://reviews.llvm.org/D35484 > >> > > >> > We have code in Chromium that does exactly this: > >> > > >> > gles2_cmd_format.h does #pragma pack(push, 4) and then #includes a > >> > file with some generated structs, with the intention that the pragma > >> > applies to them. > >> > > >> > What's the best way to pacify the warning in this case? > >> > > >> > (We're tracking this in > >> > https://bugs.chromium.org/p/chromium/issues/detail?id=749197) > >> > >> I agree that cases 1) and 3) from your patch description make sense to > >> warn for, but I'm not sure that's the case for 2). Do you have > >> examples where this catches any bugs? In our case #pragma packing an > >> included file is intentional, and I suspect it might be a bit of a > >> pattern. > > > > > > I see, thanks for your input. > > > > 2) is generally designed for times when #pragma pack pop was accidentally > > used too late (after some #includes that unintentionally receive the > > alignment). I can see how some projects use this pattern heavily, and I > > don't think there's a good way to pacify this warning in that case. > > > > I think that for us it would be reasonable to turn 2) off by default, and > > allow users to enable it explicitly using a stronger flag (something like > > -Wpragma-pack-suspicious-include?). I think that I will leave 2) out of > this > > commit, recommit it without 2) and then commit 2) as a non-default > warning > > that uses a separate flag. > > That sounds reasonable. You can probably still do it with the same > commit, just moving 2) behind a separate flag. > > Thanks, > Hans >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
