Re: Remove unnecessary else branch
Li Japin 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
Re: Remove unnecessary else branch
On Oct 13, 2020, at 9:59 PM, Hamid Akhtar mailto:hamid.akh...@gmail.com>> wrote: On Tue, Oct 13, 2020 at 6:37 PM Heikki Linnakangas mailto:hlinn...@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
On Oct 13, 2020, at 9:59 PM, Hamid Akhtar mailto:hamid.akh...@gmail.com>> wrote: On Tue, Oct 13, 2020 at 6:37 PM Heikki Linnakangas mailto:hlinn...@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
On Tue, Oct 13, 2020 at 6:37 PM Heikki Linnakangas 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
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
Remove unnecessary else branch
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