Thread: Remove unnecessary else branch

Remove unnecessary else branch

From
Li Japin
Date:
Hi,

I found in guc-file.l we can omit the else branch in AbsoluteConfigLocation().

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index c98e220295..9d4b3d7236 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -522,23 +522,21 @@ AbsoluteConfigLocation(const char *location, const char *calling_file)

        if (is_absolute_path(location))
                return pstrdup(location);
+
+       if (calling_file != NULL)
+       {
+               strlcpy(abs_path, calling_file, sizeof(abs_path));
+               get_parent_directory(abs_path);
+               join_path_components(abs_path, abs_path, location);
+               canonicalize_path(abs_path);
+       }
        else
        {
-               if (calling_file != NULL)
-               {
-                       strlcpy(abs_path, calling_file, sizeof(abs_path));
-                       get_parent_directory(abs_path);
-                       join_path_components(abs_path, abs_path, location);
-                       canonicalize_path(abs_path);
-               }
-               else
-               {
-                       AssertState(DataDir);
-                       join_path_components(abs_path, DataDir, location);
-                       canonicalize_path(abs_path);
-               }
-               return pstrdup(abs_path);
+               AssertState(DataDir);
+               join_path_components(abs_path, DataDir, location);
+               canonicalize_path(abs_path);
        }
+       return pstrdup(abs_path);
 }


--
Best regards
Japin Li






Re: Remove unnecessary else branch

From
Heikki Linnakangas
Date:
On 13/10/2020 16:30, Li Japin wrote:
> Hi,
> 
> I found in guc-file.l we can omit the else branch in AbsoluteConfigLocation().

It will compile the same, so it's just a matter of code readability or 
taste which style is better here. I think we should leave it alone, it's 
fine as it is.

- Heikki



Re: Remove unnecessary else branch

From
Hamid Akhtar
Date:


On Tue, Oct 13, 2020 at 6:37 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 13/10/2020 16:30, Li Japin wrote:
> Hi,
>
> I found in guc-file.l we can omit the else branch in AbsoluteConfigLocation().

It will compile the same, so it's just a matter of code readability or
taste which style is better here. I think we should leave it alone, it's
fine as it is.

- Heikki



I agree with Heikki from the code execution point of view. 

"canonicalize_path(abs_path);" statement is also condition independent and can be pulled out of both if and else blocks. Removing unnecessary statements makes the code more readable, but it is a matter of choice/style.

Re: Remove unnecessary else branch

From
Li Japin
Date:

On Oct 13, 2020, at 9:59 PM, Hamid Akhtar <hamid.akhtar@gmail.com> wrote:



On Tue, Oct 13, 2020 at 6:37 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 13/10/2020 16:30, Li Japin wrote:
> Hi,
>
> I found in guc-file.l we can omit the else branch in AbsoluteConfigLocation().

It will compile the same, so it's just a matter of code readability or
taste which style is better here. I think we should leave it alone, it's
fine as it is.

- Heikki



I agree with Heikki from the code execution point of view. 

In code execution point of view they are same, however, the code is for user, i think the readability is also important.


"canonicalize_path(abs_path);" statement is also condition independent and can be pulled out of both if and else blocks. Removing unnecessary statements makes the code more readable, but it is a matter of choice/style.

+1

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index c98e220295..b3549665ef 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -522,23 +522,21 @@ AbsoluteConfigLocation(const char *location, const char *calling_file)

        if (is_absolute_path(location))
                return pstrdup(location);
+
+       if (calling_file != NULL)
+       {
+               strlcpy(abs_path, calling_file, sizeof(abs_path));
+               get_parent_directory(abs_path);
+               join_path_components(abs_path, abs_path, location);
+       }
        else
        {
-               if (calling_file != NULL)
-               {
-                       strlcpy(abs_path, calling_file, sizeof(abs_path));
-                       get_parent_directory(abs_path);
-                       join_path_components(abs_path, abs_path, location);
-                       canonicalize_path(abs_path);
-               }
-               else
-               {
-                       AssertState(DataDir);
-                       join_path_components(abs_path, DataDir, location);
-                       canonicalize_path(abs_path);
-               }
-               return pstrdup(abs_path);
+               AssertState(DataDir);
+               join_path_components(abs_path, DataDir, location);
        }
+
+       canonicalize_path(abs_path);
+       return pstrdup(abs_path);
 }

--
Best regards
Japin Li

Re: Remove unnecessary else branch

From
Li Japin
Date:

On Oct 13, 2020, at 9:59 PM, Hamid Akhtar <hamid.akhtar@gmail.com> wrote:



On Tue, Oct 13, 2020 at 6:37 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 13/10/2020 16:30, Li Japin wrote:
> Hi,
>
> I found in guc-file.l we can omit the else branch in AbsoluteConfigLocation().

It will compile the same, so it's just a matter of code readability or
taste which style is better here. I think we should leave it alone, it's
fine as it is.

- Heikki



I agree with Heikki from the code execution point of view. 

In code execution point of view they are same, however, the code is for user, i think the readability is also important.


"canonicalize_path(abs_path);" statement is also condition independent and can be pulled out of both if and else blocks. Removing unnecessary statements makes the code more readable, but it is a matter of choice/style.

+1

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index c98e220295..b3549665ef 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -522,23 +522,21 @@ AbsoluteConfigLocation(const char *location, const char *calling_file)

        if (is_absolute_path(location))
                return pstrdup(location);
+
+       if (calling_file != NULL)
+       {
+               strlcpy(abs_path, calling_file, sizeof(abs_path));
+               get_parent_directory(abs_path);
+               join_path_components(abs_path, abs_path, location);
+       }
        else
        {
-               if (calling_file != NULL)
-               {
-                       strlcpy(abs_path, calling_file, sizeof(abs_path));
-                       get_parent_directory(abs_path);
-                       join_path_components(abs_path, abs_path, location);
-                       canonicalize_path(abs_path);
-               }
-               else
-               {
-                       AssertState(DataDir);
-                       join_path_components(abs_path, DataDir, location);
-                       canonicalize_path(abs_path);
-               }
-               return pstrdup(abs_path);
+               AssertState(DataDir);
+               join_path_components(abs_path, DataDir, location);
        }
+
+       canonicalize_path(abs_path);
+       return pstrdup(abs_path);
 }

--
Best regards
Japin Li

Re: Remove unnecessary else branch

From
Tom Lane
Date:
Li Japin <japinli@hotmail.com> writes:
> I agree with Heikki from the code execution point of view.

> In code execution point of view they are same, however, the code is for user, i think the readability is also
important.

There is another consideration here, which is avoiding creating
back-patching hazards from gratuitous cross-branch code differences.

If you need to rewrite a chunk of logic anyway, then fixing
small cosmetic issues in it is fine.  Otherwise I think "leave
well enough alone" is a good guiding principle.

            regards, tom lane