Re: [PATCH 2/3] Factor out duplicate code in gimplify_scan_omp_clauses

[Jakub Jelinek <jakub at redhat dot com>]

On Sat, Nov 10, 2018 at 09:11:19AM -0800, Julian Brown wrote:
> This patch, created while trying to figure out the open-coded linked-list
> handling in gimplify_scan_omp_clauses, factors out four somewhat
> repetitive portions of that function into two new outlined functions.
> This was done largely mechanically; the actual lines of executed code are
> more-or-less the same.  That means the interfaces to the new functions
> is somewhat eccentric though, and could no doubt be improved.  I've tried
> to add commentary to the best of my understanding, but suggestions for
> improvements are welcome!
> 
> As a bonus, one apparent bug introduced during an earlier refactoring
> to use the polynomial types has been fixed (I think!): "known_eq (o1,
> 2)" should have been "known_eq (o1, o2)".
> 
> Tested alongside other patches in this series and the async patches. OK?
> 
> ChangeLog
> 
> 	gcc/
> 	* gimplify.c (insert_struct_component_mapping)
> 	(check_base_and_compare_lt): New.

I think
	* gimplify.c (insert_struct_component_mapping,
	check_base_and_compare_lt): New.
is what is used far more often than the above syntax.

> +
> +static tree
> +insert_struct_component_mapping (enum tree_code code, tree c, tree struct_node,
> +				 tree prev_node, tree *scp)

Please use a shorter name, like insert_struct_comp_mapping or even
insert_struct_comp_map, to avoid formatting glitches.

> +{
> +  enum gomp_map_kind mkind = (code == OMP_TARGET_EXIT_DATA
> +			      || code == OACC_EXIT_DATA)
> +			     ? GOMP_MAP_RELEASE : GOMP_MAP_ALLOC;

Please use
  enum gomp_map_kind mkind
    = ((code == OMP_TARGET_EXIT_DATA || code == OACC_EXIT_DATA)
       ? GOMP_MAP_RELEASE : GOMP_MAP_ALLOC);
instead.

> +		  int base_eq_orig_base
> +		    = check_base_and_compare_lt (OMP_CLAUSE_DECL (c),
> +			&orig_base, decl, &bitpos1, &offset1);

Incorrect formatting, &orig_base needs to be below OMP_CLAUSE_DECL.  So:
		  int base_eq_orig_base
		    = check_base_and_compare_lt (OMP_CLAUSE_DECL (c),
						 &orig_base, decl, &bitpos1,
						 &offset1);

> +			    int same_decl_offset_lt
> +			      = check_base_and_compare_lt (
> +				  OMP_CLAUSE_DECL (*sc), NULL, decl,
> +				  &bitpos1, &offset1);
> +			    if (same_decl_offset_lt == -1)

Again, wrong formatting.  If even the first argument doesn't fit, just use
a temporary.
			    tree sc_decl = OMP_CLAUSE_DECL (*sc);
			    int same_decl_offset_lt
			      = check_base_and_compare_lt (sc_decl, NULL, decl,
							   &bitpos1, &offset1);

> +			  tree cl
> +			    = insert_struct_component_mapping (code, c, NULL,
> +				*prev_list_p, scp);

Also wrong formatting, should be:

			  tree cl
			    = insert_struct_component_mapping (code, c, NULL,
							       *prev_list_p,
							       scp);

or if the name is shorter, you can fit more.

>  			  if (sc == prev_list_p)
>  			    {
>  			      *sc = cl;

Otherwise LGTM, but I admit I haven't verified every single statement.

	Jakub