r335084 - Append new attributes to the end of an AttributeList.

[Richard Smith via cfe-commits]

On 23 June 2018 at 22:34, Michael Kruse via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> Hi,
>
> multiple comments in the code indicate that the attribute order was
> surprising and probably has lead to bugs, and will lead to bugs in the
> future. The order had to be explicitly reversed to avoid those. This
> alone for me this seems a good reason to have the attributes in the
> order in which they appear in the source.
>
> It would be nice it you could send a reproducer. At a glance, your
> case does not seem related since the format strings are function
> arguments, not attributes.
>
> Michael
>
>
> 2018-06-23 17:17 GMT-05:00 David Jones <dlj at google.com>:
> > This commit seems to have some substantial downsides... for example, it
> now
> > flags calls like this as warnings:
> > http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-
> tools/src/msgl-check.c?id=05ecefa943f339019b7127aa92cbb09f6fd49ed3#n478
> >
> > Previously, the reverse order meant that the plural format string was
> > examined, but now it is only the singular string. Since the singular
> string
> > doesn't include a substitution, the unused format variable warning fires
> > after this revision.
> >
> > I don't see a strong argument for why one order is more correct than the
> > other; however, given that this is in conflict with real code found in
> the
> > wild, I think this needs to be addressed.
> >
> > Since the semantics of the ordering of multiple format args seems
> somewhat
> > ill-defined, it seems to me like reverting may be the best near-term
> choice.
> > I can imagine other ways to fix the diagnostic, but the only behaviour
> that
> > I would believe to be expected by existing code is the old one, so a
> change
> > like this one probably needs to be more carefully vetted before being
> > (re-)committed.
>
> Could you give more details about your concerns?


(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.)

> Please let me know your plan. (If I don't hear back in a day or so, I'll
> go
> > ahead and revert for you as the safe default course of action.)
>
> On a weekend?


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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180625/4f3c75b3/attachment.html>