On 28-10-14 01:40, Linus Torvalds wrote:
On Mon, Oct 27, 2014 at 3:57 PM, Jef Driesen jef@libdivecomputer.org wrote:
On 23-10-14 00:34, Linus Torvalds wrote:
Both the windows and non-libusb builds are broken.
I have no sane way to test the windows build issues. But I fixed the non-libusb build by moving the ifdef around a bit, and I renamed "error()" to "report_error()" to hopefully get the Windows build fixed.
Can you mention the actual broken casts? This is my #1 "C++ is a broken piece opf shit" moment, though, since I really hate how *wrong* C++ got the whole "void *" thing (not just NULL, absolutely _everything_ is broken wrt type handling of "void *" in C++).
Basically every cast from void* to something else. In C++ you need an explicit cast, while in C this isn't necessary.
I admit that this whole C++ hack is annoying, and I would rather ignore it. But on Windows supporting msvc is pretty much a must. I don't use it at all myself (I cross compile on Linux), other than checking once in a while whether it still compiles. But there are people using it. And if a few extra casts are necessary to make this work, then that's a price I'm willing to pay.
I don't expect you to fix all Windows issues, but if you can already keep this in mind, that means less work afterwards.
There are also some header files like unistd.h which don't exist on windows. Several of the headers you include are not necessary at all, and can be removed. There are other things like the snprintf function which doesn't implement the C99 standard and returns a different value if the buffer is too short.
But even the mingw (gcc) build fails. It fails on the ERROR macros with "error: called object '0' is not a function". Not sure why.
It sounds like the mingw header files use some odd kind of "error" #define. Worked around by making it be "report_error()"
I remember again what's the problem. The libusb header pulls in the windows.h header. And somewhere deep in there, there is a ERROR macro defined. I worked around that in the past by doing:
#ifdef HAVE_LIBUSB #ifdef _WIN32 #define NOGDI #endif #include <libusb-1.0/libusb.h> #endif
We don't need GDI at all, so it's no problem to disable it.
The directory_entry structure with it's zero length array is most likely also not very portable. C99 has flexible arrays, but I'm not sure what msvc supports.
Changed to a one-byte array - it's going to waste a few bytes of memory per entry, but it's not a big deal. (That's not due to the "1"
- that could be easily fixed - but due to then also aligning the thing
up for "sizeof()").
Better integration with the existing libdivecomputer infrastructure. We already have logging functions, array_uint{16,24,32}_{be,le} helper functions for dealing with endianness, dynamic memory buffers (dc_buffer_t), etc. There is no need to re-implement any of those.
Quite frankly, your logging functions suck, because they don't return any value. So you can't use "return ERROR()", which is the sane common use.
Plus they need that "context" thing which makes no sense and isn't even available except at "open" time.
It's not just open that wants to log things.
So no, I can't use that horrible thing. For example, even ignoring the brokenness of the "context" thing, something like:
if (badness) return report_error("badness %d happened", badness);compared to:
if (badness) { ERROR(context, "Badness %d happened", badness); return DC_STATUS_WHATEVER; }is the difference between "I can bother to make reasonable error reporting" and "f*ck this, I'll not bother logging the reason at all". So then I'd just do
if (badness) return DC_STATUS_FAIL;
so now there's no logging at all - just because your interface is bad.
After the open call, the context pointer is stored in the device/parser structure, and you can get it from there.
Libdivecomputer doesn't log anything on its own. The actual logging is done by the application. That's on purpose. That's for example how subsurface is able to generate a libdivecomputer log directly from the GUI. I believe that's a very useful feature, which certainly doesn't sucks :-)
It may require a bit more verbosity in our code, but it saves me *a lot* of time when dealing with support issues. So it's worth the effort. Trust me, you don't want to explain some Windows user how to redirect stderr to a file. In a Windows GUI app you don't even have stderr! So this is not something I'm willing to change.
But I can't even use that DC_STATUS_FAIL kind of thing. See later.
As to the "array_uint()" functions - you have no writing functions, and you're lacking a "u8". And your types are wrong, not allowing "void *". And they are insanely verbose. But whatever. I converted the ones you had.
Well, nobody said you may not add new functions.
Why do you need void pointers here? You're parsing a byte array, which is by definition an unsigned char array. So where do you get void pointers from?
Out of memory error handling. I know the arguments for calling exit very well, but for libdivecomputer I made the choice to handle out of memory differently. Please stick to this pattern.
I made it use the dc_buffer_t, and just ignore out-of-memory cases. They don't happen, and if they did, they'd just result it empty buffers now.
That's fine. The dc_buffer_t functions handle out of memory internally.
Many functions return -1 in the case of an error. Can you rework this as dc_status_t values? BTW, if you want to use goto style error handling, that's fine.
No, I cannot use dc_status_t values, because they make it impossible to return an error *and* a value.
There is a very good reason why UNIX calling conventions are "return -1 on error".
That reason is the *non-error* case. Think of a simple function like "read()". Ask yourself what it returns on error, and what it returns on success.
Yes, I have used things like DC_STATUS_SUCCESS. I used VMS for some time. It's horrible. Suddenly you have to first check for success, and then have another interface to return the result size etc.
The whole "negative means error, zero or positive is success" is really powerful. It means that you can return how many bytes you read, and easily test whether there was something wrong, *without* having to have some "pass return size value as a pointer" model.
So please realize that my "send_receive()" function works like "read()" - it returns negative on error, and the number of bytes received on success.
I can't do that with DC_STATUS_XYZ.
I'm not going to argue which style is better or not. There are valid arguments for both styles, and you can get things wrong anyway. If you forget to return value from read() for negative values, things can go very wrong, and so on.
The point is that I deliberately choose to not mix error codes and values in libdivecomputer. I have nothing against your implementation on its own. I'm only trying to maintain a consistent style throughout the entire codebase. If every backend starts to do things in a completely different way, then the end result becomes a mess. That's all. Nothing more, nothing less.
But the main reason why I mention the -1 in the first place is that in most places you just return -1 to indicate a failure, while in some case you could give a more meaningful error. And then we're back at the dc_status_t values, because those are the "native" libdivecomputer error codes.
BTW, one of the nice things about enums, is that whenever I see dc_status_t I immediately know it contains some status value. With plain int I need some more context. Is it success/failure (0 or -1), true/false (1 vs 0) or some value? Also in a debugger you get to see the symbolic name, not some random number.
For the name of the backend, I prefer a single word like "eonsteel" or just "steel" instead of eon_steel. This is just to follow the existing practice in libdivecomputer were backends are named DC_FAMILY_VENDOR_PRODUCT. So no underscores in the product part.
Did it for DC_FAMILY_SUUNTO_EONSTEEL.
Main structures (struct eon_steel, eon_parser) and all entry point functions (public or through the vtable) get the same backend prefix.
I *really* don't want to do this, though. The public ones are "suunto_eon_steel()" because they are visible to others. But I really object to writing that long name out for internal helper functions that are static. Why write that horribly long name for no gain?
I don't insist on it for all functions, only the entry point functions (e.g. the ones that end up in the vtable). The reason behind that is when running in the debugger it makes it easy to set breakpoints if the functions have a standard naming pattern.
For the main structures, they extend the base structure, so I indicate that in the name. For example suunto_eonsteel_device_t extends the base dc_device_t structure.
The parser logic is a bit hard to follow without having some example data to look at. Can you share some data files?
I don't have any data files, I just run it on the dive computer. Remember: this is not a "memory dump" style device.
So the dc_device_dump() approach doesn't work, because on this dive computer you'd have to do it per-dive.
It would be easy to dump the parse data, but I don't think there is any such functionality: you expect the dump to be a memory dump of the whole device.
I know it doesn't support a standard memory dump. But a dump of one dive is fine too. I can use the parser standalone. That's actually something I want to address with the new dctool application. There are other devices that don't support memory dumps. And for debugging the parsing code it would be very convenient to be able to work with single dives. That's even easier than the simulator.
Jef