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.

Sign up for GitHub

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

Jump to bottom

Fixed #397: add C++ support #554

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixed #397: add C++ support #554

wants to merge 2 commits into from

Conversation

willson-chen
Copy link
Member

original patch by Galik. Related PR #398

@vapier
Copy link
Member

vapier commented Nov 27, 2019

afaict, this is the same as the previous PR, which means none of the concerns have been addressed. please see the discussion in #397.

@willson-chen
Copy link
Member Author

willson-chen commented Nov 28, 2019 •
edited

@vapier hi, two questions:

  • should we support cpp in GD-2.3? by inistalling gd_io_stream.h and related files.
  • we add gdpp.cxx and gd_io_stream.cxx to src/CMakeLists.txt, but exclude them in src/Makefile.am. what is the reason?

thx!

@vapier
Copy link
Member

vapier commented Nov 28, 2019

we should have a --enable-cxx option to configure.ac so people can control it

would also be nice if we could link the code into libgd w/out requiring libstdc++, but i'm not sure how easy that'll be to pull off

@willson-chen
Copy link
Member Author

willson-chen commented Nov 29, 2019 •
edited

we should have a --enable-cxx option to configure.ac so people can control it

easy to implement. but could you please tell me the reason? although most libgd users do not need c++ wrapper, integrating libgd with c++ without any option sounds resonable considering the relation between c and c++.

and reference to my question 2, we should include gdpp.cxx and gd_io_stream.cxx ONLY when option --enable-cxx or -DENABLE_CXX is supplied, right?

would also be nice if we could link the code into libgd w/out requiring libstdc++, but i'm not sure how easy that'll be to pull off

we use iostream in gd_io_stream.h, and it depends on libstdc++. so how can we make libgd independent of libstdc++?

@willson-chen
Copy link
Member Author

@vapier , hi, i added --enable-cxx option in my last commit. is it what we want? please have a review.

thx

@vapier
Copy link
Member

vapier commented Dec 10, 2019

integrating libgd with c++ without any option sounds resonable considering the relation between c and c++.

sorry but I don't know what you mean by relation.

libgd today does not require a C++ compiler or C++ runtime. that's good for embedded situations that don't have C++ compilers, and even on regular systems where we don't waste resources loading large and normally unused C++ runtimes.

a common idiom in other projects seems to provide in an entirely sep lib like libgdxx.so or libgdcpp.so. that would hold all the C++ bits and not penalize everyone else.

@willson-chen
Copy link
Member Author

willson-chen commented Dec 11, 2019 •
edited

integrating libgd with c++ without any option sounds resonable considering the relation between c and c++.

sorry but I don't know what you mean by relation.

libgd today does not require a C++ compiler or C++ runtime. that's good for embedded situations that don't have C++ compilers, and even on regular systems where we don't waste resources loading large and normally unused C++ runtimes.

a common idiom in other projects seems to provide in an entirely sep lib like libgdxx.so or libgdcpp.so. that would hold all the C++ bits and not penalize everyone else.

Hi, @vapier .

So a better way to support C++ is to generate libgdcpp.so when option --enable-cxx is on?

If so, is libgdcpp.so the superset of libgd.so, or libgdcpp.so depends on libgd.so?

vapier
vapier requested changes Mar 3, 2021
View reviewed changes
Copy link
Member

@vapier vapier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So a better way to support C++ is to generate libgdcpp.so when option --enable-cxx is on?

correct!

If so, is libgdcpp.so the superset of libgd.so, or libgdcpp.so depends on libgd.so?

it would depend on libgd.so

CMakeLists.txt
@@ -10,6 +10,7 @@ SET(CMAKE_MODULE_PATH "${GD_SOURCE_DIR}/cmake/modules")
include(gd)

OPTION(ENABLE_GD_FORMATS "Enable GD image formats" 0)
OPTION(ENABLE_CXX "Enable CPP wrapper" 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"CPP wrapper" -> "C++ library"

configure.ac
@@ -113,6 +114,14 @@ fi
AC_DEFINE_UNQUOTED([ENABLE_GD_FORMATS], [$gd_ac_value], [Whether to support gd image formats])
AM_CONDITIONAL([ENABLE_GD_FORMATS], test "$gd_enable_gd_formats" = yes)

AC_MSG_CHECKING([whether to support cpp wrapper])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about "where to enable C++ interface library"

configure.ac
@@ -113,6 +114,14 @@ fi
AC_DEFINE_UNQUOTED([ENABLE_GD_FORMATS], [$gd_ac_value], [Whether to support gd image formats])
AM_CONDITIONAL([ENABLE_GD_FORMATS], test "$gd_enable_gd_formats" = yes)

AC_MSG_CHECKING([whether to support cpp wrapper])
AC_ARG_ENABLE([cxx],
[AS_HELP_STRING([--enable-cxx], [Enable cpp wrapper])],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about "Enable C++ interface library"

configure.ac
@@ -301,6 +310,7 @@ AC_MSG_RESULT([
** Configuration summary for $PACKAGE $VERSION:

Support for gd/gd2 images: $gd_enable_gd_formats
Support for cpp wrapper: $gd_enable_cxx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"CPP wrapper" -> "C++ library"

@vapier vapier changed the title Fixed #397: add cpp support Fixed #397: add C++ support Mar 3, 2021
@pierrejoye
Copy link
Contributor

I see this PR now. Please note I added the ENABLE_CPP option in CMake, so it makes perfect sense to have it in configure as well.

I do agree as well on having a libgdcpp library, depending on libgd.so

@pierrejoye
Copy link
Contributor

Are the changes done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vapier vapier vapier requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@willson-chen @vapier @pierrejoye