DevSolar / pdclib

The Public Domain C Library
https://pdclib.rootdirectory.de
Creative Commons Zero v1.0 Universal
224 stars 41 forks source link

Improve handling of `strerror` messages #9

Open JayFoxRox opened 4 years ago

JayFoxRox commented 4 years ago

(This is a low priority issue, but ports would probably require updates, so it's good to fix this before a bad implementation propagates)

As also indicated by the comment, this function is not ideal:

https://github.com/DevSolar/pdclib/blob/862f106acddd9ffb40d2fe45bb38bf1e0bc81bc7/functions/string/strerror.c#L13-L24

We actually discussed this briefly downstream in this conversation:

DevSolar commented 4 years ago

Hmm...

The benefit of having strerror() / perror() work on an array like that is that it allows for static initialization in _PDCLIB_stdinit.c. I was hoping to get around any "library initialization" plumbing, i.e. a function that would have to be called by the C runtime prior to jumping into main().

It also kept the internals of strerror() / perror() comparatively simple.

I have to think a bit about alternatives... perhaps a two-level mapping, with one array mapping the individual (host) error numbers to a consecutive sequence?

Any ideas, or preferences?

DevSolar commented 4 years ago

The source comment about "static array is not the way to go" was aimed at the original construct, which would not have allowed changing the error messages according to locale. This is almost-solved work-in-progress, which however didn't address the issue of enumeration gaps.

I think I got a solution:

I don't quite see the necessity for having the error number included in the messages generated by strerror / perror. The caller has the value of errno, and can add it to the error output if so desired. Or am I missing something?

JayFoxRox commented 4 years ago

I think I got a solution:

Yes, this was mostly was implying, too:

There'd be no pre-processing (library initialization). And on platforms that want a number as part of the error message that could be added easily without many code-changes.

I don't quite see the necessity for having the error number included in the messages generated by strerror / perror.

I think it's still useful to allow this for mimicking an existing libc (similar to what the error readout tool achieves).

The caller has the value of errno, and can add it to the error output if so desired.

Manually adding it in the caller means a lot of redundant code and added complexity (as the users code suddenly has to deal with sprintf or something).

DevSolar commented 4 years ago

{Removed revision} does introduce a different structure for holding the error messages. There are no longer holes in _PDCLIB_messages_t. This however required to add the errno value to the array (as the index no longer corresponds to that), and adding some logic to find the correct enrty. (I also added a bit extra so that, if more than one symbol is defined to the same value, always the message for the first symbol is returned.)

In the end, this increases code size, by about 300 bytes on my machine for a release build. (And that is without the code that would add the errno value to the "Unknown Error" message.)

At which point I'd be tempted to roll back that revision. Any thoughts?


Edit: To avoid confusion, I have rolled back that last SVN revision for now. A patch tarball is available at http://rootdirectory.ddns.net/static/pdclib_path_strerror.tgz if you want to try some things.

I think a patch that actually increases memory usage wouldn't satisfy anyway, correct?

DevSolar commented 4 years ago

...on second thought, I guess I over-engineered that approach a little. I could write a switch in _PDCLIB_geterrtext() that transposes the non-continuous errno values into continuous index values, and stick with the direct index lookup into _PDCLIB_lc_messages...

I will give that a shot, possibly this weekend.