| Message ID | 20200526142124.36202-2-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 3 small cleanups for find_first_block_group | expand |
On 26.05.20 г. 17:21 ч., Johannes Thumshirn wrote: > The 'out' label in find_first_block_group() does not do anything in terms > of cleanup. > > It is better to directly return 'ret' instead of jumping to out to not > confuse readers. Additionally there is no need to initialize ret with 0. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> I personally prefer returning fast aka the way you've done it but dunno if David is a fan of this. In any case: Reviewed-by: Nikolay Borisov <nborisov@suse.com>
On Tue, May 26, 2020 at 06:24:49PM +0300, Nikolay Borisov wrote: > > > On 26.05.20 г. 17:21 ч., Johannes Thumshirn wrote: > > The 'out' label in find_first_block_group() does not do anything in terms > > of cleanup. > > > > It is better to directly return 'ret' instead of jumping to out to not > > confuse readers. Additionally there is no need to initialize ret with 0. > > > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > I personally prefer returning fast aka the way you've done it but dunno > if David is a fan of this. In any case: I'm not and the pattern that should be used is a mix of both. The first part is for the 'obvious' cases: Early exit from function can use return, eg. when a feature is not enabled, when there's no cleanup needed, when the return is inside an if and is not nested For the rest it's recommended to use the goto and single return as it may be a big chunk of code with a lot of nesting and a return somewhere in the middle reads harder. In example of find_first_block_group the first 'goto out' after btrfs_search_slot would be a candidate to convert to return, but the rest is inside a while loop so goto is preferred. It is also important to keep one style and switch to it eventually and I think that the goto + single return is quite common nowadays, exceptions exist in the old code.
diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c index 176e8a292fd1..c5acd89c864c 100644 --- a/fs/btrfs/block-group.c +++ b/fs/btrfs/block-group.c @@ -1527,7 +1527,7 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info, struct btrfs_key *key) { struct btrfs_root *root = fs_info->extent_root; - int ret = 0; + int ret; struct btrfs_key found_key; struct extent_buffer *leaf; struct btrfs_block_group_item bg; @@ -1536,7 +1536,7 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info, ret = btrfs_search_slot(NULL, root, key, path, 0, 0); if (ret < 0) - goto out; + return ret; while (1) { slot = path->slots[0]; @@ -1546,7 +1546,7 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info, if (ret == 0) continue; if (ret < 0) - goto out; + return ret; break; } btrfs_item_key_to_cpu(leaf, &found_key, slot); @@ -1594,11 +1594,11 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info, } } free_extent_map(em); - goto out; + return ret; } path->slots[0]++; } -out: + return ret; }
The 'out' label in find_first_block_group() does not do anything in terms of cleanup. It is better to directly return 'ret' instead of jumping to out to not confuse readers. Additionally there is no need to initialize ret with 0. Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/btrfs/block-group.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)