Skip to content

Conversation

@eile
Copy link

@eile eile commented Nov 10, 2016

No description provided.

Copy link
Member

@deanberris deanberris left a comment

Choose a reason for hiding this comment

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

Can you explain why this is necessary, and whether it's possible for you to make these options instead of forcing the defaults?

option( CPP-NETLIB_BUILD_SHARED_LIBS "Build cpp-netlib as shared libraries." ON )
option( CPP-NETLIB_BUILD_TESTS "Build the cpp-netlib project tests." OFF)
option( CPP-NETLIB_BUILD_EXPERIMENTS "Build the cpp-netlib project experiments." OFF)
option( CPP-NETLIB_BUILD_EXAMPLES "Build the cpp-netlib project examples." OFF)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not comfortable with changing these defaults. Is there a reason why you don't set these from your project?

system::error_code error;
tcp::resolver resolver(service_);
tcp::resolver::query query(address_, port_);
tcp::resolver::query query(tcp::v4(), address_, port_);
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be no reason for this -- can you not make this explicit in your code instead? As part of the options?

@deanberris
Copy link
Member

Hi @eile -- I sent some comments a while back and I'm unclear as to why this is necessary and why this isn't an optional thing (as opposed to something that forces IPv4). Do you mind making it so that we can configure what kind of IP addresses we'd like to get from the DNS would be? Then I can go ahead and merge the updated version.

@eile
Copy link
Author

eile commented Dec 15, 2016

Hi @deanberris: @tribal-tec and me where a bit swamped lately, we'll get back to you on this.

@deanberris
Copy link
Member

All good, thanks @eile -- no rush. :)

@deanberris
Copy link
Member

Also by the way, I just noticed that you're attempting to make a change to the 0.11-devel branch, which we aren't actively maintaining anymore. We're trying to get everyone to use the 0.13-release branch instead. I'm happy to still merge an updated version of this in 0.11-devel, but I'm not really up for having another release on the 0.11.x line.

@dnachbaur
Copy link

Please close this in favor of #729

@eile eile closed this Feb 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants