Re: [PATCH 1/3] Encapsulate comp_cost within a class with methods.

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Thu, May 19, 2016 at 11:23 AM, Martin LiÅka <mliska@suse.cz> wrote:
> On 05/16/2016 03:55 PM, Martin LiÅka wrote:
>> On 05/16/2016 12:13 PM, Bin.Cheng wrote:
>>> Hi Martin,
>>> Could you please rebase this patch and the profiling one against
>>> latest trunk?  The third patch was applied before these two now.
>>>
>>> Thanks,
>>> bin
>>
>> Hello.
>>
>> Sending the rebased version of the patch.
>>
>> Martin
>>
>
> Hello.
>
> As I've dramatically changed the 2/3 PATCH, a class encapsulation is not needed any longer.
> Thus, I've reduced this patch just to usage of member function/operators that are useful
> in my eyes. It's up the Bin whether to merge the patch?
Yes, I think we want c++-ify such structures.

> +comp_cost
> +operator- (comp_cost cost1, comp_cost cost2)
> +{
> +  if (cost1.infinite_cost_p () || cost2.infinite_cost_p ())
> +    return comp_cost::get_infinite ();
> +
> +  cost1.cost -= cost2.cost;
> +  cost1.complexity -= cost2.complexity;
> +
> +  return cost1;
> +}
For subtraction, should we expect the second operand as infinite?
Maybe add an assertion for it in case anything goes wrong here.

> +comp_cost
> +comp_cost::get_infinite ()
> +{
> +  return comp_cost (INFTY, INFTY);
> +}
> +
> +comp_cost
> +comp_cost::get_no_cost ()
> +{
> +  return comp_cost ();
> +}
I think we may keep the original global variables for
no_cost&infinite_cost, and save these two methods.
>
> @@ -5982,11 +6083,11 @@ iv_ca_recount_cost (struct ivopts_data *data, struct iv_ca *ivs)
>  {
>    comp_cost cost = ivs->cand_use_cost;
>
> -  cost.cost += ivs->cand_cost;
> +  cost+= ivs->cand_cost;
Space.

This is pure refactoring, could you please make sure there is no falls
out by simply comparing SPEC code generation/disassembly?  I am asking
since cost computation is sensitive, last time we didn't catch a "*"
character typo in dump info improvement patch.

Okay with above changes, unless somebody else has comment on the C++
part (which I know very little about).

Thanks,
bin
>
> Martin