Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use find_package and target_link_libraries to include system double conversion and properly include double-conversion #579

Open
Neumann-A opened this Issue Mar 8, 2019 · 1 comment

Comments

Projects
None yet
3 participants
@Neumann-A
Copy link

Neumann-A commented Mar 8, 2019

Description

double-conversion exports configs and targets. Those should be used to correctly include/link against double-conversion.

Expected coding style

in ITK\Modules\Core\Common\src\itkNumberToString.cxx
#include "double-conversion/double-conversion.h"
(actually would expect #include <double-conversion/double-conversion.h> for system headers
but that is against ITK coding style)

In ITK\Modules\ThirdParty\DoubleConversion\CMakeLists.txt

if(ITK_USE_SYSTEM_DOUBLECONVERSION)
  find_package(double-conversion CONFIG REQUIRED)
else()

and a target_link_libraries call where the target which requires double-conversion is linked against target double-conversion::double-conversion (Properly in ITK\Modules\Core\Common\src\CMakeLists.txt @ line 138)

Actual coding style

in ITK\Modules\Core\Common\src\itkNumberToString.cxx
#include "double-conversion.h"

In ITK\Modules\ThirdParty\DoubleConversion\CMakeLists.txt

if(ITK_USE_SYSTEM_DOUBLECONVERSION)
  find_library(double-conversion_LIBRARIES double-conversion)
  find_path(double-conversion_INCLUDE_DIRS double-conversion.h
    PATH_SUFFIXES double-conversion
    )

  if (double-conversion_LIBRARIES AND double-conversion_INCLUDE_DIRS)
    set(ITKDoubleConversion_SYSTEM_INCLUDE_DIRS
      ${double-conversion_INCLUDE_DIRS})
    set(ITKDoubleConversion_LIBRARIES
      "${double-conversion_LIBRARIES}")
  else()
    message(ERROR "double-conversion system library not found")
  endif()
else()

@Neumann-A Neumann-A added the type:Style label Mar 8, 2019

@dzenanz

This comment has been minimized.

Copy link
Member

dzenanz commented Mar 8, 2019

This was recently touched by #366, and Microsoft/vcpkg#5115 relies on it. And #include <double-conversion/double-conversion.h> was the problematic part. As this issue includes a lot of suggestions, do you mind turning it into a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.