On 03/14/2014 02:27 PM, Jiri Malak wrote:
> I enclosed both patches.

Thanks.  Here are some more comments.

The version extraction logic is currently:

> +   /* __WATCOMC__ = VVRR + 1100 Open Watcom */
> +#  define COMPILER_VERSION_MAJOR DEC((__WATCOMC__ - 1100 ) / 100)
> +#  define COMPILER_VERSION_MINOR DEC((__WATCOMC__ / 10) % 10)

The comment does not match the implementation.  It looks like
the least-significant digit is not part of the minor version
number according to the implementation.  The comment should
then read "VVR0" instead of "VVRR".  What is the purpose of
the least-significant digit in Open Watcom?  Is it a patch level?

I see several updates of the form:

> -if ("${CMAKE_CXX_COMPILER_ID}" MATCHES Watcom)
> +if ("${CMAKE_CXX_COMPILER_ID}" MATCHES Watcom
> +    OR "${CMAKE_CXX_COMPILER_ID}" MATCHES OpenWatcom)

but the MATCHES test is already a regex so you do not need
to test both variants.  Most of these updates can be dropped.

There is a lingering problem from the old code that this
hunk indents:

> +    if("${_compiler_version}" LESS 1.7)
> +      set(WATCOM16 1)
> +    endif()
> +    if("${_compiler_version}" EQUAL 1.7)
> +      set(WATCOM17 1)
> +    endif()
> +    if("${_compiler_version}" EQUAL 1.8)
> +      set(WATCOM18 1)
> +    endif()
> +    if("${_compiler_version}" EQUAL 1.9)
> +      set(WATCOM19 1)
> +    endif()

The version comparisons should use VERSION_LESS and
VERSION_EQUAL to get component-wise tests.

In InstallRequiredSystemLibraries:

> +  if(_compiler_id MATCHES "Watcom")
> +    math(EXPR _watcom_minor "${_watcom_minor} / 10")

should use STREQUAL instead of MATCHES or OpenWatcom will match.

Thanks,
-Brad

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers

Reply via email to