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

Add primal_internal.h #194

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

amontoison
Copy link
Member

@amontoison amontoison commented Apr 24, 2024

primal_internal.h contains all headers for the C interface.
It includes prima.h such that we can only write the exported routines in primal.h and the unexported ones in primal_internal.h.

@amontoison
Copy link
Member Author

amontoison commented Apr 24, 2024

@zaikunzhang We have new errors on Windows with the Intel compilers.
All tests passed on the other platforms.

@zaikunzhang
Copy link
Member

Thank you @amontoison . The errors complain that some function is deprecated and suggest a replacement. It seems easy to fix, right? Thanks.

@amontoison
Copy link
Member Author

amontoison commented Apr 26, 2024

Thank you @amontoison . The errors complain that some function is deprecated and suggest a replacement. It seems easy to fix, right? Thanks.

For the issue with deprecated routine, we can use the preprocessed macro _WIN32 to check if we are on Windows.

For the alignment error, I don't know what we should do.

c/tests/stress.c Fixed Show fixed Hide fixed
c/tests/stress.c Fixed Show fixed Hide fixed
c/tests/stress.c Fixed Show fixed Hide fixed
@@ -65,8 +72,9 @@
// Set the random seed to year/week
char buf[10] = {0};
time_t t = time(NULL);
struct tm *tmp = localtime(&t);
int rc = strftime(buf, 10, "%y%W", tmp);
struct tm timeinfo;

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error test

timeinfo is not a recognized word. (unrecognized-spelling)
struct tm *tmp = localtime(&t);
int rc = strftime(buf, 10, "%y%W", tmp);
struct tm timeinfo;
localtime_safe(&timeinfo, &t);

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error test

timeinfo is not a recognized word. (unrecognized-spelling)
int rc = strftime(buf, 10, "%y%W", tmp);
struct tm timeinfo;
localtime_safe(&timeinfo, &t);
int rc = strftime(buf, 10, "%y%W", &timeinfo);

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error test

timeinfo is not a recognized word. (unrecognized-spelling)
@zaikunzhang
Copy link
Member

zaikunzhang commented Apr 30, 2024

Hi @amontoison ,

For the issue with deprecated routine, we can use the preprocessed macro _WIN32 to check if we are on Windows.

// Thread-safe version of localtime
#ifdef _WIN32
#define localtime_safe(a, b) localtime_s(a, b)
#else
#define localtime_safe(a, b) localtime_r(b, a)
#endif

Why do we need to differentiate _WIN32 from others? Could we just use localtime_s in both cases (I note that localtime_r is available only with C23, which is too recent)?

In addition, why are a and b reversed in the two cases?

Thanks.

Zaikun

@zaikunzhang
Copy link
Member

https://github.com/amontoison/prima/blob/c_headers_internal/c/tests/stress.c#L10

// Make PRIMA available
#include "prima/prima_internal.h"

Shouldn't we use prima.h instead of prima_internal.h here?

@amontoison
Copy link
Member Author

amontoison commented Apr 30, 2024

https://github.com/amontoison/prima/blob/c_headers_internal/c/tests/stress.c#L10

// Make PRIMA available
#include "prima/prima_internal.h"

Shouldn't we use prima.h instead of prima_internal.h here?

You need the header of get_random_sed in stress.c and I don't think that you want to export it.

@amontoison
Copy link
Member Author

amontoison commented Apr 30, 2024

Hi @amontoison ,

For the issue with deprecated routine, we can use the preprocessed macro _WIN32 to check if we are on Windows.

// Thread-safe version of localtime
#ifdef _WIN32
#define localtime_safe(a, b) localtime_s(a, b)
#else
#define localtime_safe(a, b) localtime_r(b, a)
#endif

Why do we need to differentiate _WIN32 from others? Could we just use localtime_s in both cases (I note that localtime_r is available only with C23, which is too recent)?

In addition, why are a and b reversed in the two cases?

Thanks.

Zaikun

Hi @zaikunzhang, I checked online and it seems that localtime_s is only available on Windows.
If localtime_s is too recent, we should keep localtime on the other platform. We don’t have warnings if a thread-safe version is not used.

@zaikunzhang
Copy link
Member

Hi @amontoison ,

I wanted to try fixing other warnings but did not know how to do it cleanly on top of your PR, so I created a new one. I hope you do not mind. See #196 .

Hi @zaikunzhang, I checked online and it seems that localtime_s is only available on Windows.

I checked and I agree.

You need the header of get_random_sed in stress.c and I don't think that you want to export it.

I changed its definition to static unsigned int ... and removed it from prima_internal.h.

Could you review #196 ? Thank you.

Zaikun

@amontoison
Copy link
Member Author

Hi @amontoison ,

I wanted to try fixing other warnings but did not know how to do it cleanly on top of your PR, so I created a new one. I hope you do not mind. See #196 .

Hi @zaikunzhang, I checked online and it seems that localtime_s is only available on Windows.

I checked and I agree.

You need the header of get_random_sed in stress.c and I don't think that you want to export it.

I changed its definition to static unsigned int ... and removed it from prima_internal.h.

Could you review #196 ? Thank you.

Zaikun

@zaikunzhang
Do you still want to keep localtime_safe(a, b) localtime_r(b, a) for platforms that are not Windows?
You can use localtime_safe(a, b) localtime(a, b) if C23 is a constraint.

@zaikunzhang
Copy link
Member

@zaikunzhang Do you still want to keep localtime_safe(a, b) localtime_r(b, a) for platforms that are not Windows? You can use localtime_safe(a, b) localtime(a, b) if C23 is a constraint.

Hi @amontoison ,

It turns out that localtime_s is not available on macOS. Since it is only for test, there is no big problem to use C23. Thank you.

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.

2 participants