On 08/05/2016 07:24 PM, David Malcolm wrote:
On Thu, 2016-08-04 at 14:22 -0600, Jeff Law wrote:
On 08/04/2016 01:24 PM, David Malcolm wrote:
Do you realize that this isn't used for ~700 lines after this
point?
Is
there any sensible way to factor some code here to avoid the
coding
disconnect. I realize the function was huge before you got in
here,
but
if at all possible, I'd like to see a bit of cleanup.
I think this is OK after that cleanup.
format_chars can get modified in numerous places in the intervening
lines, which is why I stash the value there.
Yea, I figured that was the case. I first noticed the stashed value,
but didn't see where it was used for far longer than I expected.
I can do some kind of cleanup of check_format_info_main, maybe
splitting out the things in the body of loop, moving them to
support
functions.
That's essentially what I was thinking.
That said, I note that Martin's sprintf patch:
https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00056.html
also touches those ~700 lines in check_format_info_main in over a
dozen
places. Given that, would you prefer I do the cleanup before or
after
the substring_loc patch?
I think you should go first with the cleanup. It'll cause Martin
some
heartburn, but that happens sometimes.
And FWIW, if you hadn't needed to stash away that value I probably
wouldn't have noticed how badly that function (and the loop in
particular) needed some refactoring.
jeff
Here's a cleanup of check_format_info_main, which introduces three
new classes to hold state, and moves code from the loop into
methods of those classes, reducing the loop from ~700 lines to
~100 lines.
Unfortunately, so much changes in this patch that the before/after
diff is hard to read. If you like the end-result, but would prefer
better history I could try to split this up into a more readable set
of patches. (I have a version of that, but they're messy)
Successfully bootstrapped®rtested the updated patch on
x86_64-pc-linux-gnu.
OK for trunk?
gcc/c-family/ChangeLog:
* c-format.c (class flag_chars_t): New class.
(struct length_modifier): New struct.
(class argument_parser): New class.
(flag_chars_t::flag_chars_t): New ctor.
(flag_chars_t::has_char_p): New method.
(flag_chars_t::add_char): New method.
(flag_chars_t::validate): New method.
(flag_chars_t::get_alloc_flag): New method.
(flag_chars_t::assignment_suppression_p): New method.
(argument_parser::argument_parser): New ctor.
(argument_parser::read_any_dollar): New method.
(argument_parser::read_format_flags): New method.
(argument_parser::read_any_format_width): New method.
(argument_parser::read_any_format_left_precision): New method.
(argument_parser::read_any_format_precision): New method.
(argument_parser::handle_alloc_chars): New method.
(argument_parser::read_any_length_modifier): New method.
(argument_parser::read_any_other_modifier): New method.
(argument_parser::find_format_char_info): New method.
(argument_parser::validate_flag_pairs): New method.
(argument_parser::give_y2k_warnings): New method.
(argument_parser::parse_any_scan_set): New method.
(argument_parser::handle_conversions): New method.
(argument_parser::check_argument_type): New method.
(check_format_info_main): Introduce classes argument_parser
and flag_chars_t, moving the code within the loop into methods
of these classes. Make various locals "const".
OK. Thanks for cleaning this up.
jeff
