prima icon indicating copy to clipboard operation
prima copied to clipboard

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

Open zaikunzhang opened this issue 9 months ago • 6 comments

This includes the fixes proposed by @amontoison in https://github.com/libprima/prima/pull/194 to fix https://github.com/libprima/prima/issues/193

zaikunzhang avatar Apr 30 '24 17:04 zaikunzhang

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 avatar Apr 30 '24 18:04 zaikunzhang

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 avatar May 01 '24 02:05 zaikunzhang

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.

amontoison avatar May 01 '24 03:05 amontoison

@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.

zaikunzhang avatar May 01 '24 03:05 zaikunzhang

@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

amontoison avatar May 01 '24 03:05 amontoison

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.

zaikunzhang avatar May 02 '24 21:05 zaikunzhang