Skip to content

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

3 participants