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
>From 77e1e42c815b2b4fcd6c1b990751f77d563f136b Mon Sep 17 00:00:00 2001
Message-Id: <77e1e42c815b2b4fcd6c1b990751f77d563f136b.1433349888.git.brad.k...@kitware.com>
From: Eon Jeong <eonik...@gmail.com>
Date: Sun, 31 May 2015 14:16:48 +0900
Subject: [PATCH 1/2] FindBISON: Use CMAKE_PARSE_ARGUMENTS to parse arguments
---
Modules/FindBISON.cmake | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/Modules/FindBISON.cmake b/Modules/FindBISON.cmake
index ec3ee78..a683d4e 100644
--- a/Modules/FindBISON.cmake
+++ b/Modules/FindBISON.cmake
@@ -74,6 +74,8 @@
find_program(BISON_EXECUTABLE NAMES bison win_bison DOC "path to the bison executable")
mark_as_advanced(BISON_EXECUTABLE)
+include(CMakeParseArguments)
+
if(BISON_EXECUTABLE)
# the bison commands should be executed with the C locale, otherwise
# the message (which are parsed) may be translated
@@ -136,27 +138,30 @@ if(BISON_EXECUTABLE)
set(BISON_TARGET_output_header "")
set(BISON_TARGET_cmdopt "")
set(BISON_TARGET_outputs "${BisonOutput}")
- if(NOT ${ARGC} EQUAL 3 AND NOT ${ARGC} EQUAL 5 AND NOT ${ARGC} EQUAL 7)
+
+ # Parsing parameters
+ set(BISON_TARGET_PARAM_OPTIONS)
+ set(BISON_TARGET_PARAM_ONE_VALUE_KEYWORDS
+ VERBOSE
+ COMPILE_FLAGS
+ )
+ set(BISON_TARGET_PARAM_MULTI_VALUE_KEYWORDS)
+ cmake_parse_arguments(
+ BISON_TARGET_ARG
+ "${BISON_TARGET_PARAM_OPTIONS}"
+ "${BISON_TARGET_PARAM_ONE_VALUE_KEYWORDS}"
+ "${BISON_TARGET_PARAM_MULTI_VALUE_KEYWORDS}"
+ ${ARGN}
+ )
+
+ if("${BISON_TARGET_ARG_UNPARSED_ARGUMENTS}" STRGREATER "")
message(SEND_ERROR "Usage")
else()
- # Parsing parameters
- if(${ARGC} GREATER 5 OR ${ARGC} EQUAL 5)
- if("${ARGV3}" STREQUAL "VERBOSE")
- BISON_TARGET_option_verbose(${Name} ${BisonOutput} "${ARGV4}")
- endif()
- if("${ARGV3}" STREQUAL "COMPILE_FLAGS")
- BISON_TARGET_option_extraopts("${ARGV4}")
- endif()
+ if("${BISON_TARGET_ARG_VERBOSE}" STRGREATER "")
+ BISON_TARGET_option_verbose(${Name} ${BisonOutput} "${BISON_TARGET_ARG_VERBOSE}")
endif()
-
- if(${ARGC} EQUAL 7)
- if("${ARGV5}" STREQUAL "VERBOSE")
- BISON_TARGET_option_verbose(${Name} ${BisonOutput} "${ARGV6}")
- endif()
-
- if("${ARGV5}" STREQUAL "COMPILE_FLAGS")
- BISON_TARGET_option_extraopts("${ARGV6}")
- endif()
+ if("${BISON_TARGET_ARG_COMPILE_FLAGS}" STRGREATER "")
+ BISON_TARGET_option_extraopts("${BISON_TARGET_ARG_COMPILE_FLAGS}")
endif()
# Header's name generated by bison (see option -d)
--
2.1.4
>From 9e59cd77f45312712370b151563621ff97f23ae5 Mon Sep 17 00:00:00 2001
Message-Id: <9e59cd77f45312712370b151563621ff97f23ae5.1433349888.git.brad.k...@kitware.com>
In-Reply-To: <77e1e42c815b2b4fcd6c1b990751f77d563f136b.1433349888.git.brad.k...@kitware.com>
References: <77e1e42c815b2b4fcd6c1b990751f77d563f136b.1433349888.git.brad.k...@kitware.com>
From: Eon Jeong <eonik...@gmail.com>
Date: Thu, 28 May 2015 10:41:37 +0900
Subject: [PATCH 2/2] FindBISON: Add DEFINES_FILE option to pass --defines=FILE
While --defines option can be given via COMPILE_FLAGS, doing it messes
the whole build process; the internal BISON_${Name}_OUTPUT_HEADER and
following BISON_${Name}_OUTPUT is determined by the default name (-d),
not from the manually given name, so the dependency checker runs the
BISON_TARGET macro again and again.
---
Modules/FindBISON.cmake | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
mode change 100644 => 100755 Modules/FindBISON.cmake
diff --git a/Modules/FindBISON.cmake b/Modules/FindBISON.cmake
old mode 100644
new mode 100755
index a683d4e..344ed99
--- a/Modules/FindBISON.cmake
+++ b/Modules/FindBISON.cmake
@@ -130,6 +130,12 @@ if(BISON_EXECUTABLE)
list(APPEND BISON_TARGET_cmdopt ${BISON_TARGET_extraopts})
endmacro()
+ # internal macro
+ macro(BISON_TARGET_option_defines Header)
+ set(BISON_TARGET_output_header "${Header}")
+ list(APPEND BISON_TARGET_cmdopt --defines=${BISON_TARGET_output_header})
+ endmacro()
+
#============================================================
# BISON_TARGET (public macro)
#============================================================
@@ -144,6 +150,7 @@ if(BISON_EXECUTABLE)
set(BISON_TARGET_PARAM_ONE_VALUE_KEYWORDS
VERBOSE
COMPILE_FLAGS
+ DEFINES_FILE
)
set(BISON_TARGET_PARAM_MULTI_VALUE_KEYWORDS)
cmake_parse_arguments(
@@ -163,14 +170,19 @@ if(BISON_EXECUTABLE)
if("${BISON_TARGET_ARG_COMPILE_FLAGS}" STRGREATER "")
BISON_TARGET_option_extraopts("${BISON_TARGET_ARG_COMPILE_FLAGS}")
endif()
+ if("${BISON_TARGET_ARG_DEFINES_FILE}" STRGREATER "")
+ BISON_TARGET_option_defines("${BISON_TARGET_ARG_DEFINES_FILE}")
+ endif()
- # Header's name generated by bison (see option -d)
- list(APPEND BISON_TARGET_cmdopt "-d")
- string(REGEX REPLACE "^(.*)(\\.[^.]*)$" "\\2" _fileext "${ARGV2}")
- string(REPLACE "c" "h" _fileext ${_fileext})
- string(REGEX REPLACE "^(.*)(\\.[^.]*)$" "\\1${_fileext}"
- BISON_${Name}_OUTPUT_HEADER "${ARGV2}")
- list(APPEND BISON_TARGET_outputs "${BISON_${Name}_OUTPUT_HEADER}")
+ if(${BISON_TARGET_output_header} STREQUAL "")
+ # Header's name generated by bison (see option -d)
+ list(APPEND BISON_TARGET_cmdopt "-d")
+ string(REGEX REPLACE "^(.*)(\\.[^.]*)$" "\\2" _fileext "${ARGV2}")
+ string(REPLACE "c" "h" _fileext ${_fileext})
+ string(REGEX REPLACE "^(.*)(\\.[^.]*)$" "\\1${_fileext}"
+ BISON_TARGET_output_header "${ARGV2}")
+ endif()
+ list(APPEND BISON_TARGET_outputs "${BISON_TARGET_output_header}")
add_custom_command(OUTPUT ${BISON_TARGET_outputs}
${BISON_TARGET_extraoutputs}
@@ -186,6 +198,7 @@ if(BISON_EXECUTABLE)
set(BISON_${Name}_OUTPUTS ${BISON_TARGET_outputs})
set(BISON_${Name}_COMPILE_FLAGS ${BISON_TARGET_cmdopt})
set(BISON_${Name}_OUTPUT_SOURCE "${BisonOutput}")
+ set(BISON_${Name}_OUTPUT_HEADER "${BISON_TARGET_output_header}")
endif()
endmacro()
--
2.1.4
--
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
