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
