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

[Michael Kruse via cfe-commits]

I just revert if to have further discussions (r335516)

Michael

2018-06-25 14:58 GMT-05:00 Eric Christopher <echristo at gmail.com>:
>
>
> 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