search

Nuand / bladeRF

bladeRF USB 3.0 Superspeed Software Defined Radio Source Code
http://nuand.com
Other
1.15k stars 459 forks source link

Make BLADERF_ERR_* a enum type #368

Closed victoredwardocallaghan closed 9 years ago

victoredwardocallaghan commented 9 years ago

Hi,

So while I was writing my Haskell binding I noticed: https://github.com/Nuand/bladeRF/blob/master/host/libraries/libbladeRF/include/libbladeRF.h#L62

Now would it not make sense to make this into a enum type and check BLADERF_ERR_WHATEVER rather than arbitrary numerical ints if (bladerf_open(..) != 0). I realise enum is not type-enforcing in C and is essentially just int's any way (isn't everying in C a int :dancers: ) however it is the best C can do to expose a unifed error code representation. It also helps with my binding work :p

Views on this please? Kind Regards, Edward.

jynik commented 9 years ago

Hi there Edward. I understand where you're coming from, and I've seen other APIs do this.

Personally, I don't really think there's a "right" or "wrong" as long as one is consistent. I think it's a little late in the game to go about changing our API (especially considering the recent 1.0.0 version bump to denote API stability), but your feedback is certainly appreciated. :)

We chose against using an enum for error values for the following reasons. This is not to say we chose "correctly", but I know I personally appreciate when rationale is provided...

We sought to stick with some of the traditional C *nix conventions of negative values for error codes, 0 on success, positive values used for other meaningful information such as the number of bladeRF devices found.

As you noted, in C, enums aren't strongly type-safe so the compiler doesn't really give us much in the way of warnings or errors about mismatching enums, or enums and other types. Therefore, in cases like this, I find enums to be more of just a reminder or conceptual hint to the programmers.

Since we document the return codes and parameters and expect programmers to understand the domain of return values for a particular function, we didn't see too much of a benefit of enums here.

There's also some matter of personal preference and opinion at play here (and of course I cannot claim personal preference to be "right" or "wrong"). I just personally become annoyed when using multiple APIs in a program, each which has its own set of return value enums, despite the fact that they all boil down to the same convention of "0 for success" and "negative for error". Maybe I'm unreasonable, but having multiple local variables for each "type" of return value, despite that they could all easily be represented by the same local "status" integer, just seems to convolute things more than it helps me.

So, as I said above -- did we make the "right" decision? Maybe not. However, we're fairly consistent and at this point I don't want to pull the rug out from under folks with API changes. I'm not familiar with developing Haskell bindings, but I'll certainly do my best to make recommendations where our API makes life inconvenient for you.

With that said, I'll await your response. I generally feel like closing an issue immediately seems rude. I certainly don't want to convey disrespect given that you've made a reasonable suggestion (and one that we might have immediately taken action on, had you caught us much earlier in the API design).

Thanks! Jon

victoredwardocallaghan commented 9 years ago

Hi Jon,

Thank you for your detailed response and also emotional consideration, that is very refreshing to see! I am now /very/ happy that I put my money and time into BladeRF.

I hopefully should be able to resolve any issues on the Haskell binding on my own, I am a fair amount there to finishing it in fact, just some grunt work left now.

I just wanted to make a couple of brief points on closing this; Although enum's are not type-enforcing, more "futuristic" compilers, such as clang, do give deeper diagnostics here. Further, I don't necessarily think the change would actually break the API at all, the enum's are treated int's all the same and the enum's fields can of course be initialised with the the same values.

Kind Regards, Edward.

jynik commented 9 years ago

Hi Edward,

I'm definitely a proponent of having more "treat others as you wish to be treated" in the developer world . ;)

Just wanted to quickly ACK your comments:

Although enum's are not type-enforcing, more "futuristic" compilers, such as clang, do give deeper diagnostics here.

Good point. I see that this blog post makes note of the conversion behavior with -Wconversion, which I believe we should be including when we build with -Wall and -Wextra. (If we're not, I'm a fan of adding some stricter options and fixing all resulting warnings.)

Further, I don't necessarily think the change would actually break the API at all, the enum's are treated int's all the same and the enum's fields can of course be initialised with the the same values.

I agree and think you're correct here. I'll admit that I'm more concerned (paranoid?) about:

If we were to switch to enums, I think that we might yield some confusion or nonintuitive aspects to functions that return either a negative error code or some "good" value (like in bladerf_is_fpga_configure or bladerf_get_device_list.

Had we gone with enum error codes from the beginning, I think we would have had the aforementioned values return those "good" values in output parameters.

Cheers! Jon