RE: Proposal for adding splay_tree_find (to find elements without updating the nodes).

[Aditya K <hiraditya at msn dot com>]

----------------------------------------
> Date: Sun, 15 Mar 2015 02:32:23 -0400
> From: tbsaunde@tbsaunde.org
> To: gcc@gcc.gnu.org
> Subject: Re: Proposal for adding splay_tree_find (to find elements without updating the nodes).
>
> hi,
>
> I'm only commenting on algorithmic stuff at this point, you should make
> sure this doesn't regress anything in make check. This stuff only
> effects code using omp stuff so compiling random c++ is unlikely to test
> this code at all.
>
> Also please follow the style in
> https://gcc.gnu.org/codingconventions.html
> and usually try to make new code similar to what's around it.
>
> @@ -384,7 +386,7 @@ new_omp_context (enum omp_region_type region_type)
>
> c = XCNEW (struct gimplify_omp_ctx);
> c->outer_context = gimplify_omp_ctxp;
> - c->variables = splay_tree_new (splay_tree_compare_decl_uid, 0, 0);
> + //c->variables = splay_tree_new (splay_tree_compare_decl_uid, 0, 0);
>
> I don't think this is what you want, xcnew is a calloc wrapper and
> doesn't call the ctor for gimplify_omp_ctx. For now placement new is
> probably the simplest way to get what you want.
>
Thanks for pointing this out. I'll do it the way c->privatized_types has been allocated.
e.g., by making c->variables a pointer to std::map and   c->variables = new gimplify_tree_t;


> -static void
> -delete_omp_context (struct gimplify_omp_ctx *c)
> -{
> - splay_tree_delete (c->variables);
> - delete c->privatized_types;
> - XDELETE (c);
> -}
>
> hm, why?
>
My bad, I'll restore this.

> -gimplify_adjust_omp_clauses_1 (splay_tree_node n, void *data)
> +gimplify_adjust_omp_clauses_1 (std::pair<tree, unsigned> n, void *data)
>
> You can now change the type of data from void * to const
> gimplify_adjust_omp_clauses_data *

Done!


Thanks for the feedback, they were really helpful. I have updated the patch. Please review this.
Also, although I run `make check` while compiling gcc (with bootstrap enabled), I'm not sure if 'omp' related tests were exercised.
I'm still unfamiliar with several components of gcc. Any pointers on how to ensure all tests were run, would be useful.


-Aditya




>
> thanks!
>
> Trev
>
> On Fri, Mar 13, 2015 at 07:32:03PM +0000, Aditya K wrote:
>> You're right. I'll change this to:
>>
>> /* A stable comparison functor to sort trees.  */
>> struct tree_compare_decl_uid {
>>   bool  operator ()(const tree &xa, const tree &xb) const
>>   {
>>     return DECL_UID (xa) < DECL_UID (xb);
>>   }
>> };
>>
>> New patch attached.
>>
>>
>> Thanks,
>> -Aditya
>>
>>
>> ----------------------------------------
>>> Date: Fri, 13 Mar 2015 19:02:11 +0000
>>> Subject: Re: Proposal for adding splay_tree_find (to find elements without updating the nodes).
>>> From: jwakely.gcc@gmail.com
>>> To: hiraditya@msn.com
>>> CC: richard.guenther@gmail.com; stevenb.gcc@gmail.com; gcc@gcc.gnu.org
>>>
>>> Are you sure your compare_variables functor is correct?
>>>
>>> Subtracting the two values seems very strange for a strict weak ordering.
>>>
>>> (Also "compare_variables" is a pretty poor name!)
>>
>
>

Attachment: splay.patch
Description: Binary data