[1/3] btrfs: remove pointless out label in find_first_block_group
diff mbox series

Message ID 20200526142124.36202-2-johannes.thumshirn@wdc.com
State New
Headers show
Series
  • 3 small cleanups for find_first_block_group
Related show

Commit Message

Johannes Thumshirn May 26, 2020, 2:21 p.m. UTC
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(-)

Comments

Nikolay Borisov May 26, 2020, 3:24 p.m. UTC | #1
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>
David Sterba May 27, 2020, 9:31 a.m. UTC | #2
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.

Patch
diff mbox series

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;
 }