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

Issues with includes #140

Closed
TheSil opened this issue May 18, 2018 · 5 comments
Closed

Issues with includes #140

TheSil opened this issue May 18, 2018 · 5 comments

Comments

@TheSil
Copy link
Contributor

TheSil commented May 18, 2018

First of all, this library is great, I am using it to get familiar with RTP/RTCP protocols. During that, I have run into couple minor issues related to including re_* header files, so I thought it would be worth sharing:

  1. The standard types uint32_t, etc, conflicts with stdint.h, which does not seem to be reflected anywhere in the library. Perhaps adding something like HAVE_STDINT_H would help?.

  2. Again a bit related, for C++ code the bool, true, false and inline keywords are all redefined, but that does not seem to be a good idea (and some compilers such as Visual Studio 2017 evaluate this as an error).

  3. (just a suggestion) The library relies heavily on importing just "re.h" (otherwise there are issues like missing include guards, etc), but since other includes are available as well, you can be sure people will include them. It might be worth considering either moving them out of API, or making them safe to include separately...

Apart from these I was able to use the library and it works well! (Well for now I have only simple RTP requirements, will be using it more thoroughly later).

@TheSil TheSil changed the title Including re_* headers issues Issues with includes May 18, 2018
@alfredh
Copy link
Contributor

alfredh commented May 20, 2018

hi,

your applications Makefile should include re.mk to get the same CFLAGS etc as re,
see the redemo project:

https://github.com/creytiv/redemo/blob/master/Makefile#L15

it is recommended that the application only includes <re.h>, see redemo:

https://github.com/creytiv/redemo/blob/master/src/sip_ua/sip_ua.c#L21

the redefinition of types is only a fallback solution, in case inttypes.h is not available.
make sure HAVE_INTTYPES_H is defined in your application.

either in your makefile or in your code:

#define HAVE_INTTYPES_H 1
#include <re.h>

Alfred

@TheSil
Copy link
Contributor Author

TheSil commented May 20, 2018

Hi Alfred,

thanks for the response, it helped a lot. With HAVE_INTTYPES_H and HAVE_STDBOOL_H it now almost works, although this part in re_types.h causes inline redefinition issue:

/* Needed for MS compiler */
#ifdef _MSC_VER
#define inline _inline
#endif

I believe that definition is needed in MS compilers only when compiled as C, but it causes following issue when compiled as C++: zeromq/czmq#212. The guard if (!defined (__cplusplus) may be needed. After fixing this single thing, everything works perfectly for me.

@alfredh
Copy link
Contributor

alfredh commented May 21, 2018

Would you like to submit a Pull Request for that change (inline and C++) ?

It would be great to test the change with different MS compilers.
(I dont have access to Windows machines).

@TheSil
Copy link
Contributor Author

TheSil commented May 26, 2018

Created the pull request, I have tested it only on latest Visual Studio 2017 ( I do not have older versions at the moment), but from what I remember, we have used C++ inline keyword for years (even in Visual Studio 6 which is like from 1998), and it worked fine, and for C it now works like before.

Also by the way, I mention it in the pull request comments, but the inline was not available for C because of poor C99 support, which is not the case anymore, so basically in new version you won't need the inline keywoard define at all, but it works with it as well.

@alfredh
Copy link
Contributor

alfredh commented May 28, 2018

fixed in commit f9224f3

Thanks @TheSil !

@alfredh alfredh closed this as completed May 28, 2018
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

No branches or pull requests

2 participants