Re: [RESEND PATCH 2/3] bcache: update document info

[wangyijing <wangyijing@huawei.com>]

在 2016/7/1 12:21, Coly Li 写道:
> 在 16/7/1 上午9:51, wangyijing 写道:
>> Hi Coly, thanks to your review and comments.
>>
>> Commit 77b5a08427e875 ("bcache: don't embed 'return' statements in closure macros")
>> remove the return in continue_at(), so I think we should update the document info
>> about continue_at().
>>
>> Thanks!
>> Yijing.
> 
> Hi Yijing,
> 
> The original version of continue_at() returns to caller function inside
> the macro, Jens thinks this macro breaks code execution flow implicitly,
> so he moves 'return' out of continue_at() and to follow continue_at() at
> the location where continue_at() is referenced.
> 
> So as I suggested, the original author means the code should return to
> the calling function.
> 
> But YES, I agree that the comments should be updated, because there is
> no 'return' inside macro continue_at(). We should explicitly point out
> that there should be a 'return' immediately following macro continue_at().

Yes, you are right, it's better to explicitly point out a return needed to follow continue_at()
than remove this document info, I will update this patch, thanks very much!



> 
> Thanks.
> 
> Coly
> 
> 
>> 在 2016/6/29 18:16, Coly Li 写道:
>>> 在 16/6/22 上午10:12, Yijing Wang 写道:
>>>> There is no return in continue_at(), update the documentation.
>>>>
>>>
>>> There are 2 modification of this patch. The first one is about a typo,
>>> it is correct.
>>>
>>> But I doubt your second modification is proper. The line removed in your
>>> patch is,
>>>> - * continue_at() also, critically, is a macro that returns the
>>> calling function.
>>>> - * There's good reason for this.
>>>> - *
>>>
>>> I think this is exactly what original author wants to say. It does not
>>> mean return a value, it means return to the calling function. And the
>>> bellowed lines explains the reason.
>>>
>>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>>>> ---
>>>>  drivers/md/bcache/closure.c |    2 +-
>>>>  drivers/md/bcache/closure.h |    3 ---
>>>>  2 files changed, 1 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
>>>> index 9eaf1d6..864e673 100644
>>>> --- a/drivers/md/bcache/closure.c
>>>> +++ b/drivers/md/bcache/closure.c
>>>> @@ -112,7 +112,7 @@ bool closure_wait(struct closure_waitlist *waitlist, struct closure *cl)
>>>>  EXPORT_SYMBOL(closure_wait);
>>>>  
>>>>  /**
>>>> - * closure_sync - sleep until a closure a closure has nothing left to wait on
>>>> + * closure_sync - sleep until a closure has nothing left to wait on
>>>
>>> Yes, this modification is good.
>>>
>>>>   *
>>>>   * Sleeps until the refcount hits 1 - the thread that's running the closure owns
>>>>   * the last refcount.
>>>> diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h
>>>> index 782cc2c..f51188d 100644
>>>> --- a/drivers/md/bcache/closure.h
>>>> +++ b/drivers/md/bcache/closure.h
>>>> @@ -31,9 +31,6 @@
>>>>   * passing it, as you might expect, the function to run when nothing is pending
>>>>   * and the workqueue to run that function out of.
>>>>   *
>>>> - * continue_at() also, critically, is a macro that returns the calling function.
>>>> - * There's good reason for this.
>>>> - *
>>>>   * To use safely closures asynchronously, they must always have a refcount while
>>>>   * they are running owned by the thread that is running them. Otherwise, suppose
>>>>   * you submit some bios and wish to have a function run when they all complete:
>>>>
>>>
>>>
>>
> 
> 
> .
>