I just revert if to have further discussions (r335516) Michael
2018-06-25 14:58 GMT-05:00 Eric Christopher <echri...@gmail.com>: > > > On Mon, Jun 25, 2018 at 12:21 PM Richard Smith via cfe-commits > <cfe-commits@lists.llvm.org> wrote: >> >> On 23 June 2018 at 22:34, Michael Kruse via cfe-commits >> <cfe-commits@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 <d...@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@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
