<div dir="ltr">(Sorry for the late reply...)<div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jun 25, 2018 at 2:45 PM Michael Kruse via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I just revert if to have further discussions (r335516)<br>
<br>
Michael<br>
<br>
2018-06-25 14:58 GMT-05:00 Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank" class="gmail-cremed gmail-cremed cremed">echristo@gmail.com</a>>:<br>
><br>
><br>
> On Mon, Jun 25, 2018 at 12:21 PM Richard Smith via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="gmail-cremed gmail-cremed cremed">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> On 23 June 2018 at 22:34, Michael Kruse via cfe-commits<br>
>> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="gmail-cremed gmail-cremed cremed">cfe-commits@lists.llvm.org</a>> wrote:<br>
>>><br>
>>> Hi,<br>
>>><br>
>>> multiple comments in the code indicate that the attribute order was<br>
>>> surprising and probably has lead to bugs, and will lead to bugs in the<br>
>>> future. The order had to be explicitly reversed to avoid those. This<br>
>>> alone for me this seems a good reason to have the attributes in the<br>
>>> order in which they appear in the source.<br>
>>><br>
>>> It would be nice it you could send a reproducer. At a glance, your<br>
>>> case does not seem related since the format strings are function<br>
>>> arguments, not attributes.<br>
</blockquote><div><br></div><div>Well, there is a nested function call, but the format warning applies to two different parameters: a singular phrasing string, and a plural one. A reproducer would look like this, which is almost verbatim from the link:</div><div><br></div><div><div>$ cat /tmp/repro.cc</div><div>#include <libintl.h></div><div>#include <stdio.h></div><div><br></div><div>void foo(){ printf(ngettext("one frobber", "%lu frobbers", 0UL), 0UL); }</div></div><div><div>$ <snip...>/bin/clang -Wall -c -o /dev/null /tmp/repro.cc </div><div>/tmp/repro.cc:4:66: warning: data argument not used by format string [-Wformat-extra-args]</div><div>void foo(){ printf(ngettext("one frobber", "%lu frobbers", 0UL), 0UL); }</div><div> ~~~~~~~~~~~~~ ^</div><div>1 warning generated.</div></div><div><br></div><div><br></div><div>Swapping the string params works, and an older revision yields inverse results.</div><div> <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>>> 2018-06-23 17:17 GMT-05:00 David Jones <<a href="mailto:dlj@google.com" target="_blank" class="gmail-cremed gmail-cremed cremed">dlj@google.com</a>>:<br>>>> > Since the semantics of the ordering of multiple format args seems<br>
>>> > somewhat<br>
>>> > ill-defined, it seems to me like reverting may be the best near-term<br>
>>> > choice.<br>
>>> > I can imagine other ways to fix the diagnostic, but the only behaviour<br>
>>> > that<br>
>>> > I would believe to be expected by existing code is the old one, so a<br>
>>> > change<br>
>>> > like this one probably needs to be more carefully vetted before being<br>
>>> > (re-)committed.<br>
>>><br>
>>> Could you give more details about your concerns?<br></blockquote><div><br></div><div><br></div><div>Multiple format strings (or repeating or reordering other attributes) seems like it lacks clear semantics. I can't find any particularly well-documented explanation of Clang's behaviour in these cases, which makes me suspect that most people would just "fix it until it works" and move on. GCC's documentation doesn't seem to be very decisive, either.</div><div><br></div><div>For example, you mentioned that multiple format attributes may be invalid; if so, then rejecting such cases should probably be a part of the fix. But that would require some vetting, since this (pretty clearly) doesn't mirror the current reality.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">>> (I'm not sure what the problem is, but as a data point, Sema::checkCall<br>
>> iterates over the FormatAttrs in order, so it's possible that changing the<br>
>> order may have triggered a new warning. That may be due to a pre-existing<br>
>> order-dependence bug, or it may be that we're losing an attribute here.)<br></blockquote><div><br></div><div>It's arguably a pre-existing order dependence bug in ngettext, but it would be ... well, a fairly prevalent bug, since it could churn any such calls through libintl/gettext/etc.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">>>> > Please let me know your plan. (If I don't hear back in a day or so,<br>
>>> > I'll go<br>
>>> > ahead and revert for you as the safe default course of action.)<br>
>>><br>
>>> On a weekend?<br>
>><br>
>><br>
>> Yes; our policy is generally to revert to green if a patch causes<br>
>> regressions that aren't going to be fixed in a few hours. This is generally<br>
>> good for the committer, because it lets you figure out what's wrong and deal<br>
>> with it on your own schedule rather than being pressured to fix it urgently<br>
>> because you're blocking the work of others.</blockquote><div><br></div><div>Well, the bug did repro on a weekend, so there's that. ;-)</div><div><br></div><div>My main goal was to give a tighter response timeout to account for any timezone drift (with a target of Monday ~everywhere).</div></div></div></div>