Skip to content

Commit

Permalink
remove trailing spaces from comparisons, obsolete quotes and explicit…
Browse files Browse the repository at this point in the history
… variable expansion
  • Loading branch information
dirk-thomas committed Jun 16, 2016
1 parent b37f157 commit ea36f5e
Show file tree
Hide file tree
Showing 30 changed files with 59 additions and 59 deletions.
2 changes: 1 addition & 1 deletion ament_cmake_auto/cmake/ament_auto_add_executable.cmake
Expand Up @@ -74,7 +74,7 @@ macro(ament_auto_add_executable target)
"${CMAKE_CURRENT_SOURCE_DIR}/include")
endif()
# link against other libraries of this package
if(NOT "${${PROJECT_NAME}_LIBRARIES} " STREQUAL " " AND
if(NOT ${PROJECT_NAME}_LIBRARIES STREQUAL "" AND

This comment has been minimized.

Copy link
@KubaO

KubaO Aug 26, 2021

This change is incorrect :(.

Suppose that for whatever reason ${PROJECT_NAME}_LIBRARIES variable is not defined. Guess what gets printed if you run the following script in cmake (it's complete, you can try it in cmake -E):

# The state here is as-if `unset (${PROJECT_NAME}_LIBRARIES)` was invoked.

if (${PROJECT_NAME}_LIBRARIES STREQUAL "")
  message ("the string is empty")
else ()
  message ("the string is not empty")
endif ()

Unfortunately, it will print the string is not empty. That's because foo STREQUAL "" treats foo as a variable only if it is defined. Otherwise it treats foo as a string, i.e. as if you wrote "foo" STREQUAL "". This is bad news: the meaning of the code is brittle and depends on runtime context - in a way that's very easy to get wrong.

You could think of it like this:

if (NOT foo STREQUAL "")
# by language semantics is equivalent to
if ((DEFINED foo AND NOT "${foo}" STREQUAL "") OR (NOT DEFINED foo AND NOT "foo" STREQUAL ""))
# by constant propagation is equivalent to
if ((DEFINED foo AND NOT "${foo}" STREQUAL "") OR (NOT DEFINED foo))

The only safe way of rewriting this test is: NOT "${${PROJECT_NAME}_LIBRARIES}" STREQUAL "" i.e. you can remove the spaces and that's about it.

I can submit a PR to fix all this, since this is everywhere :(

The var STREQUAL ... tests can be relatively safely written when the variable is set in local context, in the same function, and when the function is short enough that a quick glance makes it obvious that in all circumstances that variable has been set to the expected value. If that's not clear, then set (var "") does the trick. It is not the same as unset (var) in this context.

When you use "${var} STREQUAL "", the result is the same when the variable is set to an empty string or undefined.

This comment has been minimized.

Copy link
@clalancette

clalancette Aug 29, 2021

Contributor

I can't say I understand all of this off-hand. But I'll encourage you to open a pull request or issue with this information in it so that we can evaluate it. Otherwise this is likely to get lost, since this is a commit from 5 years ago.

NOT ARG_NO_TARGET_LINK_LIBRARIES)
target_link_libraries("${target}" ${${PROJECT_NAME}_LIBRARIES})
endif()
Expand Down
2 changes: 1 addition & 1 deletion ament_cmake_auto/cmake/ament_auto_add_library.cmake
Expand Up @@ -74,7 +74,7 @@ macro(ament_auto_add_library target)
"${CMAKE_CURRENT_SOURCE_DIR}/include")
endif()
# link against other libraries of this package
if(NOT "${${PROJECT_NAME}_LIBRARIES} " STREQUAL " " AND
if(NOT ${PROJECT_NAME}_LIBRARIES STREQUAL "" AND
NOT ARG_NO_TARGET_LINK_LIBRARIES)
target_link_libraries("${target}" ${${PROJECT_NAME}_LIBRARIES})
endif()
Expand Down
4 changes: 2 additions & 2 deletions ament_cmake_auto/cmake/ament_auto_package.cmake
Expand Up @@ -50,7 +50,7 @@ macro(ament_auto_package)
endif()

# export and install all libraries
if(NOT "${${PROJECT_NAME}_LIBRARIES} " STREQUAL " ")
if(NOT ${PROJECT_NAME}_LIBRARIES STREQUAL "")
ament_export_libraries(${${PROJECT_NAME}_LIBRARIES})
install(
TARGETS ${${PROJECT_NAME}_LIBRARIES}
Expand All @@ -61,7 +61,7 @@ macro(ament_auto_package)
endif()

# install all executables
if(NOT "${${PROJECT_NAME}_EXECUTABLES} " STREQUAL " ")
if(NOT ${PROJECT_NAME}_EXECUTABLES STREQUAL "")
install(
TARGETS ${${PROJECT_NAME}_EXECUTABLES}
ARCHIVE DESTINATION lib
Expand Down
2 changes: 1 addition & 1 deletion ament_cmake_core/cmake/core/ament_package.cmake
Expand Up @@ -39,7 +39,7 @@ macro(ament_package)
message(FATAL_ERROR "ament_package() PROJECT_NAME is not set. You must "
"call project() before calling ament_package().")
endif()
if("${PROJECT_NAME} " STREQUAL "Project ")
if(PROJECT_NAME STREQUAL "Project")
message(FATAL_ERROR "ament_package() PROJECT_NAME is set to 'Project', "
"which is not a valid project name. "
"You must call project() before calling ament_package().")
Expand Down
2 changes: 1 addition & 1 deletion ament_cmake_core/cmake/core/ament_package_xml.cmake
Expand Up @@ -49,7 +49,7 @@ macro(ament_package_xml)
_ament_package_xml(${CMAKE_CURRENT_BINARY_DIR}/ament_cmake_core ${ARGN})

# verify that the package name from package.xml equals the project() name
if(NOT "${_AMENT_PACKAGE_NAME} " STREQUAL "${PROJECT_NAME} ")
if(NOT _AMENT_PACKAGE_NAME STREQUAL PROJECT_NAME)
message(FATAL_ERROR "ament_package_xml() package name "
"'${_AMENT_PACKAGE_NAME}' in '${_PACKAGE_XML_DIRECTORY}/package.xml' "
"does not match current PROJECT_NAME '${PROJECT_NAME}'. "
Expand Down
8 changes: 4 additions & 4 deletions ament_cmake_core/cmake/core/normalize_path.cmake
Expand Up @@ -35,7 +35,7 @@ function(normalize_path var path)
_normalize_path__collapse_uplevel_reference(parts "${parts}")

# check if path has completely collapsed
if("${parts} " STREQUAL " ")
if(parts STREQUAL "")
set(parts ".")
endif()

Expand All @@ -56,7 +56,7 @@ function(_normalize_path__collapse_redundant var)
endif()
list(GET parts ${index} part)
# remove empty parts as well as current directory references
if("${part} " STREQUAL " " OR "${part} " STREQUAL ". ")
if(part STREQUAL "" OR part STREQUAL ".")
list(REMOVE_AT parts ${index})
else()
math(EXPR index "${index} + 1")
Expand Down Expand Up @@ -85,11 +85,11 @@ function(_normalize_path__collapse_uplevel_reference var)
# get current element
list(GET parts ${index} current)

if("${current} " STREQUAL ".. " AND NOT "${previous} " STREQUAL ".. ")
if(current STREQUAL ".." AND NOT previous STREQUAL "..")
# collapse the '..'
list(REMOVE_AT parts ${index})
set(index ${previous_index})
if("${previous} " STREQUAL " ")
if(previous STREQUAL "")
# only collapse the '..' when starting with '/../'
if(previous_index GREATER 0)
string(REPLACE ";" "/" path "${parts}")
Expand Down
2 changes: 1 addition & 1 deletion ament_cmake_core/cmake/core/string_ends_with.cmake
Expand Up @@ -29,7 +29,7 @@ function(string_ends_with str suffix var)
if(NOT ${str_length} LESS ${suffix_length})
math(EXPR str_offset "${str_length} - ${suffix_length}")
string(SUBSTRING "${str}" ${str_offset} ${suffix_length} str_suffix)
if("${str_suffix} " STREQUAL "${suffix} ")
if(str_suffix STREQUAL suffix)
set(value TRUE)
endif()
endif()
Expand Down
4 changes: 2 additions & 2 deletions ament_cmake_core/cmake/core/templates/nameConfig.cmake.in
Expand Up @@ -12,10 +12,10 @@ if(NOT @PROJECT_NAME@_FIND_QUIETLY)
endif()

# warn when using a deprecated package
if(NOT "@PACKAGE_DEPRECATED@ " STREQUAL " ")
if(NOT "@PACKAGE_DEPRECATED@" STREQUAL "")
set(_msg "Package '@PROJECT_NAME@' is deprecated")
# append custom deprecation text if available
if(NOT "@PACKAGE_DEPRECATED@ " STREQUAL "TRUE ")
if(NOT "@PACKAGE_DEPRECATED@" STREQUAL "TRUE")
set(_msg "${_msg} (@PACKAGE_DEPRECATED@)")
endif()
message(WARNING "${_msg}")
Expand Down
Expand Up @@ -49,7 +49,7 @@ function(ament_generate_package_environment)

# collect package hooks to be sourced for this extension
set(ENVIRONMENT_HOOKS "")
if(NOT "${_AMENT_CMAKE_ENVIRONMENT_HOOKS_${extension}} " STREQUAL " ")
if(NOT "${_AMENT_CMAKE_ENVIRONMENT_HOOKS_${extension}}" STREQUAL "")
list(SORT _AMENT_CMAKE_ENVIRONMENT_HOOKS_${extension})
foreach(hook ${_AMENT_CMAKE_ENVIRONMENT_HOOKS_${extension}})
set(native_hook "/${hook}")
Expand Down
4 changes: 2 additions & 2 deletions ament_cmake_core/cmake/index/ament_index_get_resource.cmake
Expand Up @@ -29,11 +29,11 @@
# @public
#
function(ament_index_get_resource var resource_type resource_name)
if("${resource_type} " STREQUAL " ")
if(resource_type STREQUAL "")
message(FATAL_ERROR
"ament_index_get_resource() called without a 'resource_type'")
endif()
if("${resource_name} " STREQUAL " ")
if(resource_name STREQUAL "")
message(FATAL_ERROR
"ament_index_get_resource() called without a 'resource_name'")
endif()
Expand Down
6 changes: 3 additions & 3 deletions ament_cmake_core/cmake/index/ament_index_get_resources.cmake
Expand Up @@ -26,7 +26,7 @@
# @public
#
function(ament_index_get_resources var resource_type)
if("${resource_type} " STREQUAL " ")
if(resource_type STREQUAL "")
message(FATAL_ERROR
"ament_index_get_resources() called without a 'resource_type'")
endif()
Expand All @@ -46,7 +46,7 @@ function(ament_index_get_resources var resource_type)
# Remove any empty strings and make sure slashes are consistent
set(paths_to_search)
foreach(path IN LISTS prefix_path)
if(NOT "${path} " STREQUAL " ")
if(NOT path STREQUAL "")
string(REPLACE "\\" "/" normalized_path "${path}")
list_append_unique(paths_to_search "${normalized_path}")
endif()
Expand All @@ -62,7 +62,7 @@ function(ament_index_get_resources var resource_type)
string(SUBSTRING "${resource}" 0 1 resource_char0)
# Ignore all subdirectories, and any files starting with a dot
if((NOT IS_DIRECTORY "${resource_index_path}/${resource_type}/${resource}")
AND (NOT "${resource_char0} " STREQUAL ". "))
AND (NOT resource_char0 STREQUAL "."))
list_append_unique(all_resources ${resource})
endif()
endforeach()
Expand Down
4 changes: 2 additions & 2 deletions ament_cmake_core/cmake/index/ament_index_has_resource.cmake
Expand Up @@ -28,11 +28,11 @@
# @public
#
function(ament_index_has_resource var resource_type resource_name)
if("${resource_type} " STREQUAL " ")
if(resource_type STREQUAL "")
message(FATAL_ERROR
"ament_index_has_resource() called without a 'resource_type'")
endif()
if("${resource_name} " STREQUAL " ")
if(resource_name STREQUAL "")
message(FATAL_ERROR
"ament_index_has_resource() called without a 'resource_name'")
endif()
Expand Down
Expand Up @@ -35,7 +35,7 @@ include(CMakeParseArguments)
# @public
#
function(ament_index_register_resource resource_type)
if("${resource_type} " STREQUAL " ")
if(resource_type STREQUAL "")
message(FATAL_ERROR
"ament_index_register_resource() called without a 'resource_type'")
endif()
Expand All @@ -51,7 +51,7 @@ function(ament_index_register_resource resource_type)
"'CONTENT' and 'CONTENT_FILE', only one is allowed")
endif()

if("${ARG_PACKAGE_NAME} " STREQUAL " ")
if(NOT ARG_PACKAGE_NAME)
set(ARG_PACKAGE_NAME "${PROJECT_NAME}")
endif()

Expand Down
Expand Up @@ -30,7 +30,7 @@ function(ament_cmake_symlink_install_directory)
endif()

# default pattern to include
if("${ARG_PATTERN} " STREQUAL " ")
if(NOT ARG_PATTERN)
set(ARG_PATTERN "*")
endif()

Expand All @@ -48,7 +48,7 @@ function(ament_cmake_symlink_install_directory)
string(LENGTH "${dir}" length)
math(EXPR offset "${length} - 1")
string(SUBSTRING "${dir}" ${offset} 1 dir_last_char)
if(NOT "${dir_last_char} " STREQUAL "/ ")
if(NOT dir_last_char STREQUAL "/")
get_filename_component(destination_name "${dir}" NAME)
set(destination "${destination}/${destination_name}")
else()
Expand All @@ -65,7 +65,7 @@ function(ament_cmake_symlink_install_directory)
RELATIVE "${dir}"
"${dir}/${pattern}"
)
if(NOT "${include_files} " STREQUAL " ")
if(NOT include_files STREQUAL "")
list(APPEND relative_files ${include_files})
endif()
endforeach()
Expand All @@ -76,7 +76,7 @@ function(ament_cmake_symlink_install_directory)
RELATIVE "${dir}"
"${dir}/${pattern}"
)
if(NOT "${exclude_files} " STREQUAL " ")
if(NOT exclude_files STREQUAL "")
list(REMOVE_ITEM relative_files ${exclude_files})
endif()
endforeach()
Expand Down Expand Up @@ -232,14 +232,14 @@ function(ament_cmake_symlink_install_targets)
# determine destination of file based on extension
set(destination "")
get_filename_component(fileext "${file}" EXT)
if("${fileext}" STREQUAL ".a" OR "${fileext}" STREQUAL ".lib")
if(fileext STREQUAL ".a" OR fileext STREQUAL ".lib")
set(destination "${ARG_ARCHIVE_DESTINATION}")
elseif("${fileext}" STREQUAL ".dylib" OR "${fileext}" STREQUAL ".so")
elseif(fileext STREQUAL ".dylib" OR fileext STREQUAL ".so")
set(destination "${ARG_LIBRARY_DESTINATION}")
elseif("${fileext} " STREQUAL " " OR "${fileext}" STREQUAL ".dll" OR "${fileext}" STREQUAL ".exe")
elseif(fileext STREQUAL "" OR fileext STREQUAL ".dll" OR fileext STREQUAL ".exe")
set(destination "${ARG_RUNTIME_DESTINATION}")
endif()
if("${destination} " STREQUAL " ")
if(destination STREQUAL "")
set(destination "${ARG_DESTINATION}")
endif()

Expand Down Expand Up @@ -274,7 +274,7 @@ function(_ament_cmake_symlink_install_create_symlink absolute_file symlink)
if(EXISTS "${symlink}" AND IS_SYMLINK "${symlink}")
get_filename_component(destination "${symlink}" REALPATH)
get_filename_component(real_absolute_file "${absolute_file}" REALPATH)
if("${destination} " STREQUAL "${real_absolute_file} ")
if(destination STREQUAL real_absolute_file)
message(STATUS "Up-to-date symlink: ${symlink}")
return()
endif()
Expand Down
Expand Up @@ -20,7 +20,7 @@
# :type ARGN: various
#
function(ament_cmake_symlink_install_directory directory_keyword)
if(NOT "${directory_keyword} " STREQUAL "DIRECTORY ")
if(NOT directory_keyword STREQUAL "DIRECTORY")
message(FATAL_ERROR "ament_cmake_symlink_install_directory() first "
"argument must be 'DIRECTORY', not '${directory_keyword}'")
endif()
Expand Down Expand Up @@ -52,11 +52,11 @@ function(ament_cmake_symlink_install_directory directory_keyword)
set(i 0)
while(i LESS length)
list(GET argn ${i} arg)
if("${arg}" STREQUAL "PATTERN")
if(arg STREQUAL "PATTERN")
math(EXPR j "${i} + 2")
if(j LESS length)
list(GET argn ${j} arg)
if("${arg}" STREQUAL "EXCLUDE")
if(arg STREQUAL "EXCLUDE")
# replace "PATTERN" with "PATTERN_EXCLUDE"
list(REMOVE_AT argn ${i})
list(INSERT argn ${i} "PATTERN_EXCLUDE")
Expand Down
Expand Up @@ -20,7 +20,7 @@
# :type ARGN: various
#
function(ament_cmake_symlink_install_files files_keyword)
if(NOT "${files_keyword} " STREQUAL "FILES ")
if(NOT files_keyword STREQUAL "FILES")
message(FATAL_ERROR "ament_cmake_symlink_install_files() first argument "
"must be 'FILES', not '${files_keyword}'")
endif()
Expand Down
Expand Up @@ -20,7 +20,7 @@
# :type ARGN: various
#
function(ament_cmake_symlink_install_programs programs_keyword)
if(NOT "${programs_keyword} " STREQUAL "PROGRAMS ")
if(NOT programs_keyword STREQUAL "PROGRAMS")
message(FATAL_ERROR "ament_cmake_symlink_install_programs() first argument "
"must be 'PROGRAMS', not '${programs_keyword}'")
endif()
Expand Down
Expand Up @@ -23,7 +23,7 @@ set(__AMENT_CMAKE_SYMLINK_INSTALL_TARGETS_INDEX "0"
# :type ARGN: various
#
function(ament_cmake_symlink_install_targets)
if(NOT "${ARGV0} " STREQUAL "TARGETS ")
if(NOT "${ARGV0}" STREQUAL "TARGETS")
message(FATAL_ERROR "ament_cmake_symlink_install_targets() first argument "
"must be 'TARGETS', not '${ARGV0}'")
endif()
Expand Down
8 changes: 4 additions & 4 deletions ament_cmake_core/cmake/symlink_install/install.cmake
Expand Up @@ -21,16 +21,16 @@
function(install signature)
string(TOUPPER "${signature}" signature)

if("${signature} " STREQUAL "DIRECTORY ")
if(signature STREQUAL "DIRECTORY")
ament_cmake_symlink_install_directory(DIRECTORY ${ARGN})
return()
elseif("${signature} " STREQUAL "FILES ")
elseif(signature STREQUAL "FILES")
ament_cmake_symlink_install_files(FILES ${ARGN})
return()
elseif("${signature} " STREQUAL "PROGRAMS ")
elseif(signature STREQUAL "PROGRAMS")
ament_cmake_symlink_install_programs(PROGRAMS ${ARGN})
return()
elseif("${signature} " STREQUAL "TARGETS ")
elseif(signature STREQUAL "TARGETS")
ament_cmake_symlink_install_targets(TARGETS ${ARGN})
return()
endif()
Expand Down
Expand Up @@ -6,16 +6,16 @@ include(CMakeParseArguments)

function(ament_cmake_uninstall_target_remove_empty_directories path)
set(install_space "@CMAKE_INSTALL_PREFIX@")
if("${install_space} " STREQUAL " ")
if(install_space STREQUAL "")
message(FATAL_ERROR "The CMAKE_INSTALL_PREFIX variable must not be empty")
endif()

string(LENGTH "${install_space}" length)
string(SUBSTRING "${path}" 0 ${length} path_prefix)
if(NOT "${path_prefix} " STREQUAL "${install_space} ")
if(NOT path_prefix STREQUAL install_space)
message(FATAL_ERROR "The path '${path}' must be within the install space '${install_space}'")
endif()
if("${path} " STREQUAL "${install_space} ")
if(path STREQUAL install_space)
return()
endif()

Expand Down
Expand Up @@ -3,7 +3,7 @@
set(_exported_definitions "@_AMENT_CMAKE_EXPORT_DEFINITIONS@")

# append definitions to @PROJECT_NAME@_DEFINITIONS
if(NOT "${_exported_definitions} " STREQUAL " ")
if(NOT _exported_definitions STREQUAL "")
foreach(_def ${_exported_definitions})
list(APPEND @PROJECT_NAME@_DEFINITIONS "${_def}")
endforeach()
Expand Down
Expand Up @@ -11,7 +11,7 @@ find_package(ament_cmake_libraries QUIET REQUIRED)
# Additionally collect the direct dependency names in
# @PROJECT_NAME@_DEPENDENCIES as well as the recursive dependency names
# in @PROJECT_NAME@_RECURSIVE_DEPENDENCIES.
if(NOT "${_exported_dependencies} " STREQUAL " ")
if(NOT _exported_dependencies STREQUAL "")
find_package(ament_cmake_core QUIET REQUIRED)
set(@PROJECT_NAME@_DEPENDENCIES ${_exported_dependencies})
set(@PROJECT_NAME@_RECURSIVE_DEPENDENCIES ${_exported_dependencies})
Expand Down
Expand Up @@ -4,7 +4,7 @@ set(_exported_include_dirs "@_AMENT_CMAKE_EXPORT_INCLUDE_DIRECTORIES@")

# append include directories to @PROJECT_NAME@_INCLUDE_DIRS
# warn about not existing paths
if(NOT "${_exported_include_dirs} " STREQUAL " ")
if(NOT _exported_include_dirs STREQUAL "")
find_package(ament_cmake_core QUIET REQUIRED)
foreach(_exported_include_dir ${_exported_include_dirs})
if(NOT IS_DIRECTORY "${_exported_include_dir}")
Expand Down
Expand Up @@ -3,7 +3,7 @@
set(_exported_interfaces "@_AMENT_CMAKE_EXPORT_INTERFACES@")

# include all exported interfaces
if(NOT "${_exported_interfaces} " STREQUAL " ")
if(NOT _exported_interfaces STREQUAL "")
foreach(_interface ${_exported_interfaces})
include("${@PROJECT_NAME@_DIR}/${_interface}Export.cmake")
endforeach()
Expand Down
Expand Up @@ -23,7 +23,7 @@ configure_file(
list(APPEND ${PROJECT_NAME}_CONFIG_EXTRAS "${_generated_extra_file}")

# install export files for interfaces
if(NOT "${_AMENT_CMAKE_EXPORT_INTERFACES} " STREQUAL " ")
if(NOT _AMENT_CMAKE_EXPORT_INTERFACES STREQUAL "")
foreach(_interface ${_AMENT_CMAKE_EXPORT_INTERFACES})
install(
EXPORT "${_interface}"
Expand Down

0 comments on commit ea36f5e

Please sign in to comment.