On Mon, Mar 16, 2015 at 4:33 AM, Aditya K <hiradi...@msn.com> wrote:
>
>
> ----------------------------------------
>> Date: Sun, 15 Mar 2015 02:32:23 -0400
>> From: tbsau...@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.
I'm not sure we want to use std::map. Can you use GCCs own hash_map
here?
Richard.
>
> -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....@gmail.com
>>>> To: hiradi...@msn.com
>>>> CC: richard.guent...@gmail.com; stevenb....@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!)
>>>
>>
>>
>
>
>
