Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builtins.cpp: #ifdef OS_NT ... #else ... #endif part: more human-readable. #69

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DoctorNoobingstoneIPresume
Copy link
Contributor

If this looks good, we can do the same for the following part:

#if defined(USE_EXECUNIX)
# include <sys/types.h>
# include <sys/wait.h>
#elif defined(OS_VMS)
# include <wait.h>
#else
/*
 * NT does not have wait() and associated macros and uses the system() return
 * value instead. Status code group are documented at:
 * http://msdn.microsoft.com/en-gb/library/ff565436.aspx
 */
# define WIFEXITED(w)  (((w) & 0XFFFFFF00) == 0)
# define WEXITSTATUS(w)(w)
#endif

=>

#if defined(USE_EXECUNIX)
    #include <sys/types.h>
    #include <sys/wait.h>
#elif defined(OS_VMS)
    #include <wait.h>
#else
    /*
     * NT does not have wait() and associated macros and uses the system() return
     * value instead. Status code group are documented at:
     * http://msdn.microsoft.com/en-gb/library/ff565436.aspx
     */
    #define WIFEXITED(w)  (((w) & 0XFFFFFF00) == 0)
    #define WEXITSTATUS(w)(w)
#endif

Copy link
Member

@grafikrobot grafikrobot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like indenting for scope, even the preprocessor logic. Unfortunately this style of indent means that, in the future, when/if we use something like clang-format it will reformat to put the # back at column 0. Hence I would prefer if the indent padding is after the #. Like this:

#if X
#   if Y
#   endif
#endif

Which is what the previous code has. It's just that it's been a really small indent, as opposed to the current preference of indenting on 4. Otherwise, the change looks fine.

@DoctorNoobingstoneIPresume
Copy link
Contributor Author

Thank you for answer ! (-:

I do like indenting for scope, even the preprocessor logic. Unfortunately this style of indent means that, in the future, when/if we use something like clang-format it will reformat to put the # back at column 0.

In https://stackoverflow.com/questions/24476165/indenting-preprocessor-directives-with-clang-format , could you please see the 2021-02-02 answer by Gabriel Staples ?

Than answer has directed me here: https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormatStyleOptions.html -- where I can "Find in page" (Ctrl+F) "BeforeHash":

PPDIS_BeforeHash (in configuration: BeforeHash) Indents directives before the hash.

#if FOO
  #if BAR
    #include <foo>
  #endif
#endif

IMO:

  • the spacing before the # enhances readability a lot;
  • the four-spaces-versus-one-space after the # enhances readability only a little bit.

But I think this is highly subjective matter. :)

Hence I would prefer if the indent padding is after the #. Like this:

#if X
#   if Y
#   endif
#endif

Which is what the previous code has. It's just that it's been a really small indent, as opposed to the current preference of indenting on 4. Otherwise, the change looks fine.

FWMOIW (For Whatever My Opinion Is Worth), if the only difference is four-spaces-versus-one-space, then perhaps we leave it with one-space => the output of git diff, git log -p etc. is not affected... :'(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants