superfluous " else if ()"

superfluous " else if ()"

[Toralf Förster]

/me wonders if this

        if (ret >= 0) {
                /* Add an item for the type for the first time */
                eb = path->nodes[0];
                slot = path->slots[0];
                offset = btrfs_item_ptr_offset(eb, slot);
        } else if (ret == -EEXIST) {
                /*
                 * An item with that type already exists.
                 * Extend the item and store the new subid at the end.
                 */
                btrfs_extend_item(uuid_root, path, sizeof(subid_le));
                eb = path->nodes[0];
                slot = path->slots[0];
                offset = btrfs_item_ptr_offset(eb, slot);
                offset += btrfs_item_size_nr(eb, slot) - sizeof(subid_le);
        } else if (ret < 0) {							<-----------------
                btrfs_warn(uuid_root->fs_info, "insert uuid item failed %d "
                        "(0x%016llx, 0x%016llx) type %u!",
                        ret, (unsigned long long)key.objectid,
                        (unsigned long long)key.offset, type);
                goto out;
        }


shouldn't be condensed into just "} else {" ?

-- 
Toralf

Re: superfluous " else if ()"

[David Sterba]

On Sat, Apr 26, 2014 at 08:57:00PM +0200, Toralf Förster wrote:
> /me wonders if this
> 
>         if (ret >= 0) {
>                 /* Add an item for the type for the first time */
>                 eb = path->nodes[0];
>                 slot = path->slots[0];
>                 offset = btrfs_item_ptr_offset(eb, slot);
>         } else if (ret == -EEXIST) {
>                 /*
>                  * An item with that type already exists.
>                  * Extend the item and store the new subid at the end.
>                  */
>                 btrfs_extend_item(uuid_root, path, sizeof(subid_le));
>                 eb = path->nodes[0];
>                 slot = path->slots[0];
>                 offset = btrfs_item_ptr_offset(eb, slot);
>                 offset += btrfs_item_size_nr(eb, slot) - sizeof(subid_le);
>         } else if (ret < 0) {							<-----------------
>                 btrfs_warn(uuid_root->fs_info, "insert uuid item failed %d "
>                         "(0x%016llx, 0x%016llx) type %u!",
>                         ret, (unsigned long long)key.objectid,
>                         (unsigned long long)key.offset, type);
>                 goto out;
>         }
> 
> 
> shouldn't be condensed into just "} else {" ?

It's equivalent to the original code, but is easier to read/understand.
Do you have there other concerns besides readability?

Re: superfluous " else if ()"

[Toralf Förster]

On 05/09/2014 06:49 PM, David Sterba wrote:
> On Sat, Apr 26, 2014 at 08:57:00PM +0200, Toralf Förster wrote:
>> /me wonders if this
>>
>>         if (ret >= 0) {
>>                 /* Add an item for the type for the first time */
>>                 eb = path->nodes[0];
>>                 slot = path->slots[0];
>>                 offset = btrfs_item_ptr_offset(eb, slot);
>>         } else if (ret == -EEXIST) {
>>                 /*
>>                  * An item with that type already exists.
>>                  * Extend the item and store the new subid at the end.
>>                  */
>>                 btrfs_extend_item(uuid_root, path, sizeof(subid_le));
>>                 eb = path->nodes[0];
>>                 slot = path->slots[0];
>>                 offset = btrfs_item_ptr_offset(eb, slot);
>>                 offset += btrfs_item_size_nr(eb, slot) - sizeof(subid_le);
>>         } else if (ret < 0) {							<-----------------
>>                 btrfs_warn(uuid_root->fs_info, "insert uuid item failed %d "
>>                         "(0x%016llx, 0x%016llx) type %u!",
>>                         ret, (unsigned long long)key.objectid,
>>                         (unsigned long long)key.offset, type);
>>                 goto out;
>>         }
>>
>>
>> shouldn't be condensed into just "} else {" ?
> 
> It's equivalent to the original code, but is easier to read/understand.
> Do you have there other concerns besides readability?
> 

Oh no, if it is intended and b/c it is not in a runtime critical path - no.

But what came into my mind, wouldn't the following avoid such question in future ? :

         } else {
		/* ret < 0 */

-- 
Toralf

Re: superfluous " else if ()"

[David Sterba]

On Fri, May 09, 2014 at 07:12:54PM +0200, Toralf Förster wrote:
> But what came into my mind, wouldn't the following avoid such question in future ? :

Yes and though it's a tiny change, I don't mind a patch for that.

>          } else {
> 		/* ret < 0 */

The comment is IMHO unnecessary.