xflouris / libpll

Phylogenetic Likelihood Library
GNU Affero General Public License v3.0
26 stars 6 forks source link

Add set_error() function #96

Closed amkozlov closed 6 years ago

amkozlov commented 8 years ago

add a function to set pll_errno and pll_errmsg simultaneously, i..e.

pll_set_error(int errno, const char* errmsg)
{
   ...
}
xflouris commented 8 years ago

@amkozlov : please avoid such functions as they will only cause confusion. The user should not be allowed to set error codes in the internal structure of the library - these codes are only return values that help the client programmer identify and resolve the error. While users are still able to change the variables manually (although they shouldn't), exporting a function that allows it, would only indicate that there is reason to have such function.

There is no reason in 'compressing' the two lines of code in the library into one function call each time an error is set, if that was your intention.

If you still need such function, please add it in the pll-modules and not in libpll.

amkozlov commented 8 years ago

@xflouris: my main intention was not to save lines (although I think code brevity is an asset), but to avoid the duplication of this

snprintf (pll_errmsg, 200, "Cannot allocate memory for bl opt variables");

where you hardcode the error message size in the dozens of places.

So in any case, I would use PLL_ERRMSG_LEN define instead.

Then, if you don't want to have such a function in libpll - I can add it in the modules as well.

But in a broader context, in the modules we might need to use some libpll functions which shouldn't be available to the end users via general API. So how about adding a special header file which will give access to some internals (i.e., more "low-level" API)?

xflouris commented 8 years ago

@amkozlov : agreed, the hardcoded value must go away. I'll replace it with a macro now that I'm standardizing and documenting all defines/errors for the public release.

I understand that there will be many 'internal' functions that are required for the modules or for the needs of the lab tools. I'd be happy if we incorporate all these in the user-code or in the pll-modules, which is code tailored for the lab needs. I'd like to keep libpll as simple and lightweight as possible.

amkozlov commented 8 years ago

@xflouris: OK, so far I added both message size macro and the wrapper function in the modules here:

https://github.com/ddarriba/pll-modules/commit/6f5c3aebc09067bb1897c0b402c73d906490a8d5

xflouris commented 8 years ago

@amkozlov : great, thanks :) Good idea with the PLLMOD prefix btw. Could you use such (or similar) syntax in pll-modules when defining macros? I've reserved the PLL_ERROR_ for error codes for libpll. Perhaps prefix all macros in pll-modules as PLL_MOD_ (i.e., PLL_MOD_ERROR_) in order to prevent clashes with libpll? I guess chances for collision are small due to the different functionality, but you never know...

amkozlov commented 8 years ago

Yes, it would be nice to have distinct prefixes for both macros and public functions (pllmod_?), also because even now it's sometimes hard to know whether e.g. pll_utree_traverse_apply is coming from the core pll or from a module.

However, this would require quite some typing work to change the existing code :)

@ddarriba what do you think?