On 11/26/2014 9:39 AM, Steven Oliver wrote:
> The patch is attached. I actually wrote it quite a while ago but never
> got around to submitting it. Please take a good look at it and let me
> know what issues you see with it.

Thanks for working on this!  Here are some comments:

* Please read the cmake-developer(7) manual section on modules:

   http://www.cmake.org/cmake/help/v3.1/manual/cmake-developer.7.html#modules

* The documentation needs to be updated to the reStructuredText system.
  See the man page for details.

* The find_library cache entry should be ICONV_LIBRARY, but may the be
  copied to ICONV_LIBRARIES for consumption by callers.
  See the man page section on standard variable names.

* The CMAKE_REQUIRED_ variable settings around the check code should
  be saved and restored in case the calling project is accumulating them.

* Does the ICONV_SECOND_ARGUMENT_IS_CONST check code actually fail to
  compile when the second argument is not const?  Some C compilers may
  just warn about the incompatible pointer type but still work.

* Please avoid trailing whitespace on lines.

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/mailman/listinfo/cmake-developers

Reply via email to