[llvm-commits] [PATCH 2/3] cmake/config-ix.cmake: Add checking ELM_Callback decl for win32.

[Óscar Fuentes]

Hello Takumi.

The patch looks formally ok. Some comments on style follow...

NAKAMURA Takumi <geek4civic at gmail.com> writes:

> ---
>  cmake/config-ix.cmake |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/cmake/config-ix.cmake b/cmake/config-ix.cmake
> index e446853..c2263ec 100755
> --- a/cmake/config-ix.cmake
> +++ b/cmake/config-ix.cmake
> @@ -366,6 +366,19 @@ else( MSVC )
>    set(LTDL_DLOPEN_DEPLIBS 0)  # TODO
>  endif( MSVC )
>  
> +if( PURE_WINDOWS )
> +  CHECK_CXX_SOURCE_COMPILES("#include <windows.h>

Start with the opening `"" and then follow on the next line, so all the
code gets the same indentation:

> +  CHECK_CXX_SOURCE_COMPILES("
> +    #include <windows.h>
> +    #include <imagehlp.h>
> +    extern \"C\" void foo(PENUMLOADED_MODULES_CALLBACK);
> +    extern \"C\" void foo(BOOL(CALLBACK*)(PCSTR,ULONG_PTR,ULONG,PVOID));
> +    int main(){return 0;}" HAVE_ELMCB_PCSTR)

Please put the result variable on its own line, so it stands out from
the chunk of code.

> +  if( HAVE_ELMCB_PCSTR )
> +    set(WIN32_ELMCB_PCSTR "PCSTR")
> +  else( HAVE_ELMCB_PCSTR )
> +    set(WIN32_ELMCB_PCSTR "PSTR")
> +  endif( HAVE_ELMCB_PCSTR )

Repeating the `if' condition in the `endif' and `else' makes the code
look noisy and gives no gain when the `if' is right there a few lines
up. This is easier to read:

> +  if( HAVE_ELMCB_PCSTR )
> +    set(WIN32_ELMCB_PCSTR "PCSTR")
> +  else()
> +    set(WIN32_ELMCB_PCSTR "PSTR")
> +  endif()

Once the `endif' is separated from the `if' by a sufficient number of
lines, repeating the condition starts to make sense, although I don't
mind if it is omitted:

> +endif( PURE_WINDOWS )
> +
>  # FIXME: Signal handler return type, currently hardcoded to 'void'
>  set(RETSIGTYPE void)
>