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