Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Each Error should be mapped to a different error code #3029

Open
PhilippGackstatter opened this issue Oct 2, 2019 · 4 comments

Comments

@PhilippGackstatter
Copy link
Contributor

commented Oct 2, 2019

As mentioned in #2979 many functions return -1 for multiple errors. Take keySetName as an example. The documentation states

Return values

size | in bytes of this new key name including ending NULL
0    | if newName is an empty string or a NULL pointer (name will be empty afterwards)
-1   | if newName is invalid (name will be empty afterwards)
-1   | if key was inserted to a keyset before

Two different errors are mapped to -1, making them indistinguishable for the caller. This is especially problematic for bindings that map these error codes to Exceptions or some Error structure. The bindings might then throw an InvalidName Exception because -1 was returned, but the real problem was that the key was in a keyset. This only leads to confusion for developers.

Error codes should probably use -1, -2, and so on (or any other mechanism that allows for explicitness) to make exact matching for the caller possible.

@markus2330 markus2330 added this to the 0.9.* milestone Oct 2, 2019
@markus2330 markus2330 added the proposal label Oct 2, 2019
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

Thank you for the proposal!

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

I agree that mapping to different negative numbers should have no negative side-effect (comparing using < 0 on "any error" is trivial) but helps in some cases (at least in the case you had in the binding and a similar case I already had in the python binding) where the different error matters.

How many places are effected (next to keySetName)?

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

How many places are effected (next to keySetName)?

Also affected

  • keyGetMeta
  • keyAddBaseName
  • keyAddName
  • keySetBaseName
  • keyGetBinary
  • keyGetString
  • keySetBinary

Affected, if null pointer and memory errors should be mapped to different error codes

  • keyCopyAllMeta
  • keyCopyMeta

Maybe affected, needs discussion

  • keyGetFullName (returns -1 on Null pointers or if maxSize is 0 or larger than SSIZE_MAX)
  • keyGetBaseName (same as above)
  • keyGetName (same as above)
  • keyString, maybe? It could return error codes instead of "(null)" and "(binary").
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

Also affected

We should fix all of them (with consistent error values).

Affected, if null pointer and memory errors should be mapped to different error codes

I think there is little benefit from this.

returns -1 on Null pointers or if maxSize is 0 or larger than SSIZE_MAX

Is both API misuse, so same return value is ok.

keyString, maybe? It could return error codes instead of "(null)" and "(binary").

This is on purpose so that a printf("%s", keyString(k)) is safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.