On 06/04/2018 07:32 AM, marxin wrote: > gcc/ChangeLog: > > 2018-06-07 Martin Liska <mli...@suse.cz> > > * tree-switch-conversion.c (MAX_CASE_BIT_TESTS): Remove. > (hoist_edge_and_branch_if_true): Likewise. > (expand_switch_using_bit_tests_p): Likewise. > (struct case_bit_test): Likewise. > (case_bit_test_cmp): Likewise. > (emit_case_bit_tests): Likewise. > (switch_conversion::switch_conversion): New class. > (struct switch_conv_info): Remove old struct. > (collect_switch_conv_info): More to ... > (switch_conversion::collect): ... this. > (check_range): Likewise. > (switch_conversion::check_range): Likewise. > (check_all_empty_except_final): Likewise. > (switch_conversion::check_all_empty_except_final): Likewise. > (check_final_bb): Likewise. > (switch_conversion::check_final_bb): Likewise. > (create_temp_arrays): Likewise. > (switch_conversion::create_temp_arrays): Likewise. > (free_temp_arrays): Likewise. > (gather_default_values): Likewise. > (switch_conversion::gather_default_values): Likewise. > (build_constructors): Likewise. > (switch_conversion::build_constructors): Likewise. > (constructor_contains_same_values_p): Likewise. > (switch_conversion::contains_same_values_p): Likewise. > (array_value_type): Likewise. > (switch_conversion::array_value_type): Likewise. > (build_one_array): Likewise. > (switch_conversion::build_one_array): Likewise. > (build_arrays): Likewise. > (switch_conversion::build_arrays): Likewise. > (gen_def_assigns): Likewise. > (switch_conversion::gen_def_assigns): Likewise. > (prune_bbs): Likewise. > (switch_conversion::prune_bbs): Likewise. > (fix_phi_nodes): Likewise. > (switch_conversion::fix_phi_nodes): Likewise. > (gen_inbound_check): Likewise. > (switch_conversion::gen_inbound_check): Likewise. > (process_switch): Use the newly created class. > (switch_conversion::expand): New. > (switch_conversion::~switch_conversion): New. > * tree-switch-conversion.h: New file. So generally this looks good.
I do note that a lot of the function comments are removed from the implementation side and moved into the header file. I personally prefer that approach, particularly for publicly visible members. However I think our coding conventions still indicate the implementation side should have the function comment. So let's keep those in place, with obvious changes since many of the arguments are now class data. During my initial high level pass over the changes it looked like various chunks of functionality were missing. It turns out they were removed in patch #1, then re-introduced in subsequent patches. That's fine, but it's slightly better from a review standpoint if such things are at least called out. > +using namespace tree_switch_conversion; I thought our coding conventions discouraged "using namespace". But after reviewing, that's for header files, so I won't object to this. This patch is fine. However, please do not install until the full set is approved. Thanks, jeff
