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

Do not add feature test macros #7

Merged
merged 2 commits into from Feb 26, 2017

Conversation

Projects
None yet
2 participants
@zeehio
Copy link
Contributor

zeehio commented Feb 23, 2017

On b4c1548 (see #5) I proposed defining _POSIX_C_SOURCE inside cutest.h in order to enable the macros to use POSIX under unix-like systems, as POSIX was used by cutest on those systems.

Here I propose to follow whatever the user wants. This means:

  • If the user explicitly defines a strict standard C compliance (e.g. compiling using --std=c99) then we do not use any POSIX feature. This is detected through the definition of the STRICT_ANSI macro.

  • If the user does not enforce any C standard, we assume POSIX is available if the system is unix-like, as before.

I believe this approach is better than the previous one because if any cutest user does not want to rely on POSIX features we won't enable them and we won't use them. However by default we use them if they are available.

zeehio added a commit to MycroftAI/mimic that referenced this pull request Feb 23, 2017

@mity

This comment has been minimized.

Copy link
Owner

mity commented Feb 24, 2017

Hi @zeehio ,

to be honest, I was thinking quite a lot before I merged the referred #5, because I disliked how the #define _POSIX_SOURCE could potentially interfere with other macros from outside (i.e. predefined by compiler command line). Furthermore I think the macro is from Linux/gcc world and many other Unix systems do not recognize it.

The current PR makes this even worse in my eyes: It makes quite a hybrid animal from cutest.h which sometimes tries to detect the predefined macros from outside and follow them and sometimes (pre)define some macro before including the system headers to change their behavior.

I would therefore highly prefer if we could unify on strictly following the environment and never setting such macro manually in the header, if that's possible.

Furthermore, as it is implemented, it also seems to change the default run-time behavior in other ways. In particular, not using fork() for the unit tests. I don't think we should interpret gcc -std=c99 as "do not use any POSIX", because it is often meant (and I believe rightfully) to just "disable GNU C extensions".

So my counter-proposal is as follows (and please let me know if it would satisfy your needs):

  1. As Linux is of course very important to support well, we should have a policy to be buildable with any options specifying C standard (newer then C90, as that really is too old, and it even lacks vsnprintf(). C89/90 simply is not supported), and that cutest.h strictly follows the environment, but does not try to set it.

  2. Remove the enforcing of _POSIX_C_SOURCE (i.e. revert #5).

  3. Remove the use of fileno(stdout) and replace it with STDOUT_FILENO from <unistd.h> (or constant 1, if we find some relevant systems do not have the macro). (The fileno() is likely the only thing which was behind #5, right?)

  4. Not accept this PR as it should not be needed anymore.

Would this work for you?

@zeehio zeehio force-pushed the zeehio:do_not_enforce_posix branch 2 times, most recently from d4c21e8 to 917e56e Feb 24, 2017

@zeehio

This comment has been minimized.

Copy link
Contributor Author

zeehio commented Feb 24, 2017

Your proposal is much better than mine.

I have updated the PR with your instructions, so you don't have to type more if you don't want to. I have given you too much work already. 😅 But feel free to do the commit yourself if you prefer.

By the way, I also disabled the test_error__ function if it is not used. When neither CUTEST_WIN__ nor CUTEST_UNIX__ are defined a "warning: unused function test_error__" appeared.

Thanks for your careful thoughts and detailed explanation.

#elif defined CUTEST_WIN__
test_colorize__ = _isatty(_fileno(stdout));
test_colorize__ = _isatty(STDOUT_FILENO);

This comment has been minimized.

@mity

mity Feb 24, 2017

Owner

For Windows, _isatty(_fileno(stdout)) should be kept. Windows SDK does not have <unistd.h>

Following @mity suggestions, use POSIX if available.
Also define test_error__ only if is used.

@zeehio zeehio force-pushed the zeehio:do_not_enforce_posix branch from 917e56e to ee38d5b Feb 26, 2017

@zeehio zeehio changed the title Do not enforce POSIX if __STRICT_ANSI__ is defined Do not add feature test macros Feb 26, 2017

@zeehio

This comment has been minimized.

Copy link
Contributor Author

zeehio commented Feb 26, 2017

Thanks for the review. Done.

@mity mity merged commit 9b4dea1 into mity:master Feb 26, 2017

@mity

This comment has been minimized.

Copy link
Owner

mity commented Feb 26, 2017

Thanks, @zeehio

@zeehio zeehio deleted the zeehio:do_not_enforce_posix branch Feb 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment