Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

[Miao Xie <miaox@cn.fujitsu.com>]

On Mon, 17 Sep 2012 18:31:13 +0200, David Sterba wrote:
> On Mon, Sep 17, 2012 at 10:21:00AM +0800, Miao Xie wrote:
>> On fri, 14 Sep 2012 15:54:18 +0200, David Sterba wrote:
>>> On Thu, Sep 13, 2012 at 06:51:36PM +0800, Miao Xie wrote:
>>>> div_factor{_fine} has been implemented for two times, cleanup it.
>>>> And I move them into a independent file named math.h because they are
>>>> common math functions.
>>>
>>> You removed the sanity checks:
>>>
>>> -       if (factor <= 0)
>>> -               return 0;
>>> -       if (factor >= 100)
>>> -               return num;
>>
>> As inline functions, they should not contain complex checks, the caller should
>> make sure the parameters are right. I think.
> 
> It's compiler's job to decide whether a function should be inlined or
> not. The keyword/function attribute 'inline' is only a hint, unless
> always_inline is used and the author should be sure that it really has
> the expected outcome and that compiler is wrong here.

Right, but I think we should make the functions as simple as possible since
they are marked as inline, because the simple function is more likely to be inlined
than the complex one.

> I don't agree that each caller should do the checks, it only makes code
> harder to read and forces the authors to check for conditions that may
> not be apparent or are just ommitted.

Right. But for these functions, we are sure the value of the parameters is
in the right range in the most place, and all the place that we are sure the
value is right is in the hot path. The only place that we need check the
parameters is in slow path, this is also the reason why we make them inline.
so doing those checks just wastes time. We just need modify the caller.

Thanks
Miao

> If we need a function that does not check the boundaries, then of course
> go for it, but I don't see such case yet.
> 
>>> in new version. And I don't think it's necessary to add an extra include
>>> with a rather generic name and trivial code. A separate .h/.c with
>>> non-filesystem related support code like this looks more suitable.
>>>
>>> Do you intend to use the functions out of extent-tree.c ?
>>
>> They are used in both extent-tree.c and volumes.c from the outset, but they
>> were implemented in these two files severally.
> 
> Ah, I see.
> 
> david
>