-
Notifications
You must be signed in to change notification settings - Fork 170
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
Public header preprocessor directives #167
Comments
Add feature detection to compile arguments and Cflags, see creytiv#167
the idea was that the application should include my goal is to reduce the number of for example the following defines might be subject to removal: HAVE_INTTYPES_H what do you think about remove these ? the goal should be that the re API is source and binary compatible, |
I'm currently including it via CMake via the use of the pkgconfig file, so I don't think so. But perhaps CMake has a Makefile compatibility layer? No idea tbh. In any case, since re has a pkgconfig file, IMHO it should be striven for that the library can be used with
should work fine and not require
That will probably resolve 90% of the issues but not all of them (like |
the pkg-config file is generated in Makefile:
so you could add all the mentioned flags to the Cflags section, would that work ? |
Could do but the defines are not prefixed so they might clash. I guess it would be an adequate compromise for now. |
@lgrahl are you planning to do any work on this ? |
Add feature detection to compile arguments and Cflags, see creytiv#167
I have added the item to the roadmap: |
In May 2014, Alfred mentioned that you don't want a configuration file to detect which features have been enabled in re at compile time and would rather have the application do runtime checks (see this mail). And I'm mostly fine with that. However, there are quite a couple of issues with that approach when including the public headers of re in an application as I will explain in the following.
HAVE_INTTYPES_H
As an application depending on re, it makes sense to include
re_types.h
pretty much everywhere instead ofinttypes.h
because it is a useful abstraction. However, at the moment the application needs to defineHAVE_INTTYPES_H
itself or it will trigger unintended behaviour/compile errors in most of the cases (e.g. defining all of those types instead of simply includinginttypes.h
).Suggested solution:
-DHAVE_INTTYPES_H
could be added to theCflags
oflibre.pc
depending on whetherinttypes.h
has been detected on the system. I'm not sure if we would also have to include-D__ssize_t_defined
for Windows but it does fall into the same category.RELEASE
The
RELEASE
definition triggers different behaviour when usingre_mbuf
. The application will most likely usembuf
structures but its build system may or may not defineRELEASE
.Suggested solution: Same as for
HAVE_INTTYPES_H
but expose-DMBUF_DEBUG=1
(ifRELEASE
was set) in theCflags
.WIN32
-relatedThere a lot of
WIN32
-related definition checks in headers. This results in...MOD_PRE
,MOD_EXT
andEXPORT_SYM
inre_mod.h
,re_net.h
andre_sa.h
, andre_types.h
.IIRC, an application building on Windows pretty much has to define
WIN32
to get anything working.Suggested solution: Expose
-DWIN32
in theCflags
if set. That might clash with other projects, though.USE_ZLIB
crc32
would be redefined ifUSE_ZLIB
is not set and zlib is being used.Suggested solution: Expose
-DUSE_ZLIB
if set.HAVE_GAI_STRERROR
While it will work fine for the library itself,
gai_strerror
will be overridden with a stub function when includingre_net.h
after having included all the necessary headers for working withgai_strerror
.Suggested solution: Adding
-DHAVE_GAI_STRERROR
(if set) toCflags
should do.HAVE_INET6
The
NET_ADDRSTRLEN
definition depends onHAVE_INET6
. It would be incorrect for the application ifHAVE_INET6
is not set.Suggested solution: Expose
-DHAVE_INET6
if set.USE_OPENSSL
A couple of OpenSSL functions and structs will be redefined when including
re_sha.h
without settingUSE_OPENSSL
.In addition,
tls_openssl_context
is not defined ifUSE_OPENSSL
is not defined.Suggested solution: Expose
-DUSE_OPENSSL
if set.VERSION
,ARCH
andOS
All of those will be redefined if not set. However, it doesn't really matter much since the
sys_*
functions exists. As such it doesn't make much sense to leave them in the public header.Suggested solution: Move them into a private header file.
SOLARIS
andHAVE_STDBOOL_H
SOLARIS
andHAVE_STDBOOL_H
trigger different behaviour inre_types.h
.Suggested solution: Expose them if they are set.
If we go with the suggested solutions, then we are really not far away from a (much more convenient) configuration header. So, it might make sense to reconsider it.
The text was updated successfully, but these errors were encountered: