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

[Eric Christopher via cfe-commits]

On Mon, Jun 25, 2018 at 12:21 PM Richard Smith via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> 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.)
>
>
FWIW I just replied to the original review thread as well - there appear to
be some warnings that are now missing that previously worked and some that
are now in reverse order from the source. I think more work might need to
happen before this patch should go back in.

-eric


> > 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.
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180625/f7cf2785/attachment.html>