Re: [PATCH] c-format.c: cleanup of check_format_info_main

[Jeff Law <law@redhat.com>]

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&regrtested 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