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

Fix https://github.com/libprima/prima/issues/193 #196

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

Conversation

zaikunzhang
Copy link
Member

@zaikunzhang zaikunzhang commented Apr 30, 2024

This includes the fixes proposed by @amontoison in #194 to fix #193

@zaikunzhang zaikunzhang changed the title Fix issue 193 Fix https://github.com/libprima/prima/issues/193 Apr 30, 2024
@zaikunzhang
Copy link
Member Author

zaikunzhang commented Apr 30, 2024

Thank you @amontoison for your approval.

Hi @nbelakovski ,

I hope to hear your opinion before merging, especially on the following lines regarding prima_rc_t.

https://github.com/libprima/prima/blob/fix_issue_193/c/prima.c#L124-L125 (cl complains that PRIMA_RESULT_INITIALIZED is not covered explicitly by a case)

https://github.com/libprima/prima/blob/fix_issue_193/c/prima.c#L152-L153 (message was set to NULL, which was suggested by me, but I now think an explicit message is better, more consistent with status, and also safer in case message is printed)

https://github.com/libprima/prima/blob/fix_issue_193/c/prima.c#L196-L199 (info was prima_rc_t, which would cause a warning of incompatible type when &info is passed to xyz_c)

https://github.com/libprima/prima/blob/fix_issue_193/c/prima.c#L237-L238 (It was return PRIMA_INVALID_INPUT; I prefer not to return in the middle; the revised version seems more consistent)

https://github.com/libprima/prima/blob/fix_issue_193/c/prima.c#L241-L245 (It was prima_get_rc_string(info) and return info)

Thank you.

@zaikunzhang
Copy link
Member Author

Hi @amontoison and @nbelakovski ,

In case no other solution is proposed, where do you think we should place prima_internal.h? I am not familiar with C conventions, but I thought include/prima was only for the public header. What do others do? I suppose we are not the first to face this situation.

Thanks.

Zaikun

@amontoison
Copy link
Member

amontoison commented May 1, 2024

Hi @amontoison and @nbelakovski ,

In case no other solution is proposed, where do you think we should place prima_internal.h? I am not familiar with C conventions, but I thought include/prima was only for the public header. What do others do? I suppose we are not the first to face this situation.

Thanks.

Zaikun

@zaikunzhang It can stay in include/prima but you just don't install it in ${prefix} folder after the compilation.

…` and `prima_init_result` static; the first handles #188
@zaikunzhang
Copy link
Member Author

@zaikunzhang It can stay in include/prima but you just don't install it in ${prefix} folder after the compilation.

Thank you @amontoison . This would necessitate changing some CMakeList.txt, right? Could you show me how to do it? I have no basic idea bout CMake. Thanks.

@amontoison
Copy link
Member

@zaikunzhang
It seems that you don't need to do something.
Only prima.h is installed:
https://github.com/libprima/prima/blob/main/c/CMakeLists.txt#L32

@zaikunzhang
Copy link
Member Author

Thank you @amontoison for your approval.

Hi @nbelakovski ,

I hope to hear your opinion before merging, especially on the following lines regarding prima_rc_t.

fix_issue_193/c/prima.c#L124-L125 (cl complains that PRIMA_RESULT_INITIALIZED is not covered explicitly by a case)

fix_issue_193/c/prima.c#L152-L153 (message was set to NULL, which was suggested by me, but I now think an explicit message is better, more consistent with status, and also safer in case message is printed)

fix_issue_193/c/prima.c#L196-L199 (info was prima_rc_t, which would cause a warning of incompatible type when &info is passed to xyz_c)

fix_issue_193/c/prima.c#L237-L238 (It was return PRIMA_INVALID_INPUT; I prefer not to return in the middle; the revised version seems more consistent)

fix_issue_193/c/prima.c#L241-L245 (It was prima_get_rc_string(info) and return info)

Thank you.

Hi @nbelakovski , do these changes look good to you? 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.

CMake test fails with strict diagnostic options for Intel compilers
2 participants