Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
DoctorNoobingstoneIPresume wants to merge 1 commit intobfgroup:main from
DoctorNoobingstoneIPresume:210802_builtins_cpp_Indentation_02h
Open

builtins.cpp: #ifdef OS_NT ... #else ... #endif part: more human-readable. #69
DoctorNoobingstoneIPresume wants to merge 1 commit intobfgroup:main from
DoctorNoobingstoneIPresume:210802_builtins_cpp_Indentation_02h

Conversation

@DoctorNoobingstoneIPresume
Copy link
Contributor

@DoctorNoobingstoneIPresume DoctorNoobingstoneIPresume commented Aug 2, 2021

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.

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

Reviewers

@grafikrobot grafikrobot grafikrobot requested changes

Assignees

No one assigned

Labels

None yet

Projects

Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /