<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 23 June 2018 at 22:34, Michael Kruse via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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>
<br>
Michael<br>
<span class=""><br>
<br>
2018-06-23 17:17 GMT-05:00 David Jones <<a href="mailto:dlj@google.com">dlj@google.com</a>>:<br>
> This commit seems to have some substantial downsides... for example, it now<br>
> flags calls like this as warnings:<br>
> <a href="http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-tools/src/msgl-check.c?id=05ecefa943f339019b7127aa92cbb09f6fd49ed3#n478" rel="noreferrer" target="_blank">http://git.savannah.gnu.org/<wbr>cgit/gettext.git/tree/gettext-<wbr>tools/src/msgl-check.c?id=<wbr>05ecefa943f339019b7127aa92cbb0<wbr>9f6fd49ed3#n478</a><br>
><br>
> Previously, the reverse order meant that the plural format string was<br>
> examined, but now it is only the singular string. Since the singular string<br>
> doesn't include a substitution, the unused format variable warning fires<br>
> after this revision.<br>
><br>
> I don't see a strong argument for why one order is more correct than the<br>
> other; however, given that this is in conflict with real code found in the<br>
> wild, I think this needs to be addressed.<br>
><br>
> Since the semantics of the ordering of multiple format args seems somewhat<br>
> ill-defined, it seems to me like reverting may be the best near-term choice.<br>
> I can imagine other ways to fix the diagnostic, but the only behaviour that<br>
> I would believe to be expected by existing code is the old one, so a change<br>
> like this one probably needs to be more carefully vetted before being<br>
> (re-)committed.<br>
<br>
</span>Could you give more details about your concerns?</blockquote><div><br></div><div>(I'm not sure what the problem is, but as a data point, Sema::checkCall iterates over the FormatAttrs in order, so it's possible that changing the order may have triggered a new warning. That may be due to a pre-existing order-dependence bug, or it may be that we're losing an attribute here.)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> Please let me know your plan. (If I don't hear back in a day or so, I'll go<br>
> ahead and revert for you as the safe default course of action.)<br>
<br>
</span>On a weekend?</blockquote><div><br></div><div>Yes; our policy is generally to revert to green if a patch causes regressions that aren't going to be fixed in a few hours. This is generally good for the committer, because it lets you figure out what's wrong and deal with it on your own schedule rather than being pressured to fix it urgently because you're blocking the work of others.</div></div></div></div>