[cmake-developers] Introduction and volunteering for the Matlab package

[Raffi Enficiaud]

I believe I addressed all the raised issues, apart from this one:

> > # WARNING: this thing pollutes the CMAKE_FIND_LIBRARY_PREFIXES global
variable. 
> > # Should it be restored afterwards? Is there a more appropriate way to
do that?
> > set(CMAKE_FIND_LIBRARY_PREFIXES ${CMAKE_FIND_LIBRARY_PREFIXES} 
> > ${_matlab_lib_prefix_for_search})

> This should be handled with a save/restore.

Are you referring to the CMakePushCheckState ? If so, please correct me but
the documentation does not state that it stores the value of
CMAKE_FIND_LIBRARY_PREFIXES. 

Tests still ok. Patch attached.

Regards,
Raffi Enficiaud

-----Message d'origine-----
De : Brad King [mailto:brad.king at kitware.com] 
Envoyé : vendredi 23 janvier 2015 21:05
À : Raffi Enficiaud
Cc : cmake-developers at cmake.org
Objet : Re: [cmake-developers] Introduction and volunteering for the Matlab
package

On 01/22/2015 11:50 AM, Raffi Enficiaud wrote:
> I can also do a pull request if you prefer,

As described in CONTRIBUTING.rst a patch here is preferred.

I've fetched your branch from https://github.com/raffienficiaud/CMake
Here are some comments.

Please wrap text in the documentation blocks of the FindMatlab module to fit
in 79 columns.

> #    find_package(Matlab
> #                 [VERSION]
> #                 [REQUIRED]
> #                 [COMPONENTS component1 [component2]])

Individual find modules don't need to summarize the find_package signature.
Documentation of components and versions can just refer to the
:command:`find_package` command and name the options.

> # .. note:
> #
> # The version above is the Matlab version...

The note text needs to be indented to be part of the note.  The same goes
for all the variable and command documentation inside explicit markup
directives like ".. variable::" and ".. command::".

> # User defined variables
> # ----------------------

This section should be called something like "Module Input Variables"
and have intro wording like "Users or projects may set the following
variables to configure this module behavior.".

> # Variables defined by the module
> # -------------------------------

This section should distinguish between cache entries and output variables.
See FindJsonCpp for an example.

> # Provided macros
> # ---------------

Generally we try to use functions with "set(... PARENT_SCOPE)".

Also on the endmacro() and endfunction() calls please drop the command name.

> # WARNING: this thing pollutes the CMAKE_FIND_LIBRARY_PREFIXES global
variable. 
> # Should it be restored afterwards? Is there a more appropriate way to do
that?
> set(CMAKE_FIND_LIBRARY_PREFIXES ${CMAKE_FIND_LIBRARY_PREFIXES} 
> ${_matlab_lib_prefix_for_search})

This should be handled with a save/restore.

> # The function arguments are:

Please use definition lists instead of bullet lists for function argument
documentation.

The copyright year should be 2014-2015 since we've spilled into that range.
There is no need for copyright notices in the Tests/RunCMake test cmake code
because there no creative input in that boilerplate.

Please remove all trailing whitespace and make sure the files are committed
with LF newlines and not CRLF newlines (including tests).
Also make sure all files end in a newline (but not trailing blank lines).

Thanks,
-Brad

-------------- next part --------------
A non-text attachment was scrubbed...
Name: findmatlab.patch
Type: application/octet-stream
Size: 257001 bytes
Desc: not available
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20150124/b84419c5/attachment-0001.obj>