So I've been going back and forth on whether or not to suggest a slight
change in how we're working.
Specifically the practice of lumping multiple fixes into a single patch
-- I realize that it's usually the case that you're finding related
issues so there's a desire to address them as a group. Doing things
that way also tends to avoid interdependent patches.
The problem is that makes reviewing more difficult and thus I'm much
less likely to knock it out as soon as it flys by my inbox. And I
think I've seen several trivial to review, but important fixes inside
larger, harder to review changes.
Let's try to keep patches to addressing one issue for the future. If
you have multiple related issues, you should consider a series of patches.
Hopefully that will cut down on the number of things are getting stalled.
Thanks for the review and the suggestion(s). I also prefer smaller
patches. I've only gotten into the habit of grouping them together
because of the cost of testing each on it own, and to streamline code
reviews (for everyone involved). But I agree that the process you'd
like to follow makes the changes more tractable.
- if (i < 0)
+ if (HOST_WIDE_INT_MIN == i)
nit. I think most folks would probably prefer this as
if (i == HOST_WIDE_INT_MIN).
HOST_WIDE_INT_MIN is a constant and when we can write an expression in
either order variable OP const is the preferred order.
You seem to be going back and forth between both styles. Let' try to
stick with variable OP const. I don't think you need to go back and fix
all of the existing const OP variable instances right now, but we may in
the future.
I learned the first style (const OP variable, and also using a less
than in favor of other relational operators) many years ago and I'm
trying to unlearn it for GCC. It's a hard habit to break.
FWIW, the const OP variable style used to be recommended to avoid
subtle bugs due to typos like 'if (var = CST)' But I realize that
with -Wparentheses warning on this there is no need for the style
when using GCC.
The 'if (X < Y)' style comes from years of writing C++ template code
that couldn't assume that any other operators were defined for the
user-defined type the template might be instantiated on. So instead
of writing 'if (X >= Y)' we'd write 'if (!(X < Y)' everywhere. All
other relational operators were taboo.
+ if (prec == HOST_WIDE_INT_MIN)
+ return fmtresult ();
Is this something that needs to be fixed now or was this a marker for
future?
I think the patch for bug 78696 resolves the FIXME (but see below).
If the FIXME was a future thing, then this is OK with the nits fixed. If
the FIXME was a marker for something you intended to address now and
just forgot, then we either need another iteration or a follow-up patch
depending on the severity of the FIXME in your mind.
The patch for bug 78696 resolves the FIXME but there will need to
be another change here to improve the handling of unknown precisions
and widths:
1) Use get_range_info to constrain non-constant width and precision,
and if that fails...
2) ...use some reasonable default (e.g., based on the precision of
the type of the directive).
Without these changes sprintf (d, "%.*f", p, 0.0) causes
warning: writing a terminating nul past the end of the destination
even at -Wformat-length=1 with no good way to suppress it. At
-Wformat-length=2, sprintf(d, "%.*i", 0) also causes a similar:
warning: ‘%.*i’ directive writing 0 or more bytes into a region...
also with no way to suppress it. (The two warnings should also be
worded the same.)
I've started to work on a general fix for both of these in my patch
for bug 78703 - -fprintf-return-value floating point handling
incorrect in locales with a mulltibyte decimal point. These fit
well together because they both separate the heuristic-based warning
byte counters from the actual counters good for optimization (which
are based on what GCC knows for certain).
Martin
