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

3 participants