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

Support for autoconf config.h #13

Merged
merged 1 commit into from May 18, 2015
Merged

Support for autoconf config.h #13

merged 1 commit into from May 18, 2015

Conversation

ghost
Copy link

@ghost ghost commented May 17, 2015

Autoconf minimises the number of compiler define (-D) flags needed by adding them to a file config.h generated by the autoheader tool. This tiny patch adds support for this mechanism (while not affecting non-autoconf builds).

@jtsiomb
Copy link
Owner

jtsiomb commented May 18, 2015

Hello!
I don't understand, what's the point of adding that conditional config.h include, if the library doesn't use autoconf to build?

Also you can't just add a config.h file willy nilly in the public header file of a library which is going to be installed system-wide. You need to also install the config.h file for that to work. But if you do that, you would also have to change installation and usage, instead of a single $(PREFIX)/include/kdtree.h file, a whole directory would be needed to avoid conflicts with other config.h files. Which would lead to changes in user programs having to include kdtree/kdtree.h instead of just kdtree.h.

I have done all that in the past if there is a benefit to it. But what would we gain in kdtree's case by all that breakage?

@ghost
Copy link
Author

ghost commented May 18, 2015

Hi John

It is just to ease the use-case where kdtree is incorporated into a project which does use autoconf, for example https://github.com/jjgreen/vfplot/blob/master/src/libkdtree/ (there I modify the c file to have the include). Note the change is not to the public header file, but to the c file, so only used to pass compile-time options to the functions therein. There's no suggestion of including or installing the config.h publicly.

It's not great amount of work for me to add the include when I update kdtree; I just thought the facility might be useful of others -- please feel free to close this if you disagree!

@jtsiomb
Copy link
Owner

jtsiomb commented May 18, 2015

My mistake, for some reason I thought it was in the header file. In kdtree.c it's pretty innocuous, and if it helps the use case of integrating into a 3rd-party project, I might as well add that.

jtsiomb added a commit that referenced this pull request May 18, 2015
@jtsiomb jtsiomb merged commit 9d39e30 into jtsiomb:master May 18, 2015
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.

None yet

1 participant