On 06/12/2018 09:41 PM, Jeff Law wrote: > 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.
Hi. Thanks for review. > > 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. I'm going to change it in order to follow our coding style. Question is whether we can change that for future? Note that there are quite long function comments in tree-switch-conversion.h. Martin > > 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 >
