| Message ID | 1466561534-17595-1-git-send-email-wangyijing@huawei.com |
|---|---|
| State | New |
| Headers | show |
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.
在 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:
>>
>
>
在 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: >>>> >>> >>> >> > > > . >
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 * * 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:
There is no return in continue_at(), update the documentation. 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(-) -- 1.7.1