On Fri, Feb 03, 2017 at 09:13:23AM -0700, Martin Sebor wrote: > I'm not opposed to the changes, certainly not to cleaning things up, > but I don't think now is the time to be making these tweaks. Jakub's > patch is fine with me without those tweaks, and with the correction
What do you mean by my patch without those tweaks? The intent of the patch is not just fix the diagnostics and wrong-code issue, but primarily that any further needed fix will need to tweak just a single spot, not many, otherwise e.g. you need to have sufficient testsuite coverage for all those paths. Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html patch, you would with my patch need just the tree_digits part, and then the very last hunk in gimple-ssa-sprintf.c (with likely_adjust && removed). Because you do the adjustments only if !res.knownrange and in that case you know argmin/argmax are actually dirtype's min/max, so 0 must be in the range. > to keep the warning (and fix the octal base prefix) that I posted > in the followup patch. (The followup patch is also necessary to > avoid incorrect optimization.) > > Regarding the printed ranges: for integer arguments the pass prints > one of two sets: > > 1) either the range the argument supplied by the caller is known to > be in, or > 2) the range synthesized by the pass for an argument in an unknown > range, or after conversion to the type of the directive > > The notes distinguish between these two by using the two phrases: > > 1) directive argument in the range [MIN, MAX] > > 2) using the range [MIN, MAX] for directive argument > > I suspect this isn't entirely consistent with all the recent changes > but that's the idea behind it. It's subtle and I'm not surprised > Jakub missed it. You still print as range [MIN, MAX] something that is in no way a range, just some values picked from the range. > I'm not opposed to changing this to make it more intuitive or useful > but again, I would rather not spend time on it now. Instead, I would > prefer to discuss what we want after the dust from the release has > settled and implement a consistent solution in GCC 8. I fear this isn't the last fix needed, the code is very complex and not sufficiently covered by testcases, so if we don't want to make the code more maintainable now, I'd be strongly for just turning -fprintf-return-value off by default for the release. Bogus warnings can be lived with or worked around, silent wrong-code is much worse. Jakub
