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

Revert "compat/strdup.h: move common compat check for strdup() to own… #285

Merged
merged 1 commit into from Nov 9, 2016

Conversation

ebassi
Copy link
Contributor

@ebassi ebassi commented Nov 7, 2016

… file"

This reverts commit aaba8c1.

This commit breaks builddir != srcdir build, but, more importantly, it
also adds a dependency on a header, "config.h", which is not installed
and it's supposed to be private — since it's generated at configuration
time and it's not meant to be used by projects compiling against a
library.

… file"

This reverts commit aaba8c1.

This commit breaks builddir != srcdir build, but, more importantly, it
also adds a dependency on a header, "config.h", which is not installed
and it's supposed to be private — since it's generated at configuration
time and it's not meant to be used by projects compiling against a
library.
@ebassi
Copy link
Contributor Author

ebassi commented Nov 7, 2016

Note: the commit I've reverted breaks the build of json-c in GNOME Continuous:

http://build.gnome.org/continuous/buildmaster/builds/2016/11/07/47/build/log-json-c.txt

I started modifying the build to add -I$(top_srcdir) -I$(top_builddir) to the project's CFLAGS, then I noticed the inclusion of "config.h" in a publicly installed header.

@hawicz hawicz merged commit 8e12f4a into json-c:master Nov 9, 2016
@commodo
Copy link
Contributor

commodo commented Nov 15, 2016

@ebassi @hawicz
Should we also add that compat-code (for strdup) in json_pointer.c now ?
I mean, I personally don't need it, but since it's in json_tokener.c and json_object.c, I'm figuring it could be added there too.

Though maybe we could reset the discussion to whether this compat code is still needed ?
Or, how far back should the compat of strdup() go ; I'm seeing that it's part of POSIX.1-2001
http://man7.org/linux/man-pages/man3/strdupa.3.html

Either way.
No strong opinion from my side.
Just raising awareness of this.

Thanks
Alex

@ebassi ebassi deleted the revert-strdup-compat branch November 15, 2016 18:55
@ebassi
Copy link
Contributor Author

ebassi commented Nov 15, 2016

If your target system does not include strdup() I'm sure you're pretty much screwed. ;-)

In general, it's perfectly fine to have a fallback; in this case, the header with the fallback code should be private, and only included inside json-c, instead of being in a public header. The strdup() fallback is meant to be used by json-c's own code, not by callers of json-c. Callers of json-c may already have their own strdup() fallback in place, which would break horribly once they include json-c.

@hawicz
Copy link
Member

hawicz commented Nov 19, 2016

The presence of strdup in POSIX is irrelevant, since the reason for this compat definition isn't to deal with old systems that don't yet have strdup, but to squash warnings on current systems when building with Visual Studio, which has taken a rather pointlessly pedantic view towards avoiding conflicts with the C standard.
See http://stackoverflow.com/questions/7582394/strdup-or-strdup/7582741#7582741 and https://msdn.microsoft.com/en-us/library/ms235384.aspx

Rather than #define strdup _strdup, it might be better to pass -D_CRT_NONSTDC_NO_DEPRECATE when building on Windows, but I don't have a good way to test whether that actually works or not.

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

3 participants