[cmake-developers] [PATCH] DEFINES_FILE feature for FindBISON module

[Brad King]

On 06/03/2015 06:26 AM, 정언 wrote:
>> Thanks!  Please re-order the patches to do the refactoring
>> of argument parsing first and the functional changes second.
>
> Can you explain what you mean by 'reordering patches'? Does it mean I
> should write one more patch for that, or I should merge my patches
> into one; or both?

Patches should each make a logically distinct change with an
explanation in the commit message.  Revising patches after
review should result in a brand new series of patches that
have corrections and replace the original patches.

I split your changes into what I meant and attached the patches as
an example.  This makes review much easier because now we can talk
about the change made by each patch instead of only the net change
made by all patches.

Given the attached patches, here are review comments:

> +    if("${BISON_TARGET_ARG_UNPARSED_ARGUMENTS}" STRGREATER "")

The if() conditions of this form are better written as

  if(NOT BISON_TARGET_ARG_UNPARSED_ARGUMENTS STREQUAL "")

Also, please update the documentation at the top of the module
to mention the new DEFINES_FILE option.

Thanks,
-Brad

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-FindBISON-Use-CMAKE_PARSE_ARGUMENTS-to-parse-argumen.patch
Type: text/x-diff
Size: 2706 bytes
Desc: not available
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20150603/beddd73d/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-FindBISON-Add-DEFINES_FILE-option-to-pass-defines-FI.patch
Type: text/x-diff
Size: 3577 bytes
Desc: not available
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20150603/beddd73d/attachment-0001.patch>