Hi,
On 06/08/2013 12:31 AM, Pete Batard wrote:
> On 2013.06.07 08:05, Ludovic Rousseau wrote:
>> It is too late to rewrite exiting and working code.
>
> How is it too late?
> This code has not been integrated and furthermore was proposed for review.
> Also, it does not work, since it breaks Windows (see below).
>
>> Unless you have a lot of free time :-)
>
> I don't, but with Windows being a demanding mistress, I don't really have
> much choice but to offer a _working_ counter proposal, which I have now
> tested on Linux, OS-X and all the Windowses (MSVC, WDK, MinGW, cygwin).
>
> And yeah, the patch itself is much larger than Hans' (though the strerror.c
> is smaller), because Hans' version ignored MSVC altogether, so the VS
> solution files and the WDK build scripts didn't reference strerror.c. This
> means that compilation with MSVC/WDK was out from the get go.
>
> On that topic, I'll add a note that if you add any source, I have to spend
> about 1/2 hour manually adding it to the MSVC/WDK solution files (very
> repetitive and boring process). Thus, since I was going to get involved
> anyway, I went for the 3 course meal.
Ugh, yes sorry about that (missing addition to the msvc project files), but
there
is not much I can do about it. Well there is one thing which I could do,
which would be to build a windows vm with the necessary msvc versions in there,
or if they cannot be parallel installed multiple vms, and then do it all
manual, but I'm sorry that is too much work for me atm.
So I'm going to keep relying on you straightening things out when there is a new
source file addition I'm afraid.
> Still I'm tempted to give a good point to Hans for trying to cater for the
> .def file... except he aliased libusb_set_locale rather than
> libusb_setlocale, so that broke MinGW (and probably cygwin too).
Oops. I guess I should do test compiles with mingw, Fedora has a full blown
mingw cross compile chain available ...
> Sorry Ludovic (and Hans), but working code this isn't -- though I should
> probably have tested and reported earlier that it wasn't.
Not working because of the project file issues, or did you find other issues
too ?
> So, while I was there, I took Ludovic's remarks into account, but went with
> my version of strerror, which I continue to think is much simpler and better
> (since it groups the localized strings together, and it'll actually break
> compilation if people add a new message and forget to add a string for the
> languages - which might very well be "SOMEONE NEED TO TRANSLATE THIS CODE" --
> With Hans' proposal, I'm willing to bet we'll get missing cases, since a case
> switch doesn't enforce jack and thus it's way too easy to miss a translation)
> and I also added French. To complete the picture, since we might
> as well test the feature, I added a new -l parameter to xusb.
And you added French translations, cool!
> You're free to cast your vote for the proposal you like best. And this too,
> just like Hans, is _only_ a proposal (albeit an actually working and tested
> one).
First of all thanks for all the project files work! We definitively want to
keep that part. As for the strerror.c changes, you make 2 distinct changes:
1) You simplify the setlocale function
But IMHO you simplify it to much, ie calling:
libusb_setlocale("Dutch");
Now will try to lookup a "du" iso code, and then if that exists do something
completely wrong. And if it does not exist, it will return
LIBUSB_ERROR_NOT_FOUND
and log a matching log message, where as the proper error + log message would be
LIBUSB_ERROR_INVALID_PARAM. Where by the first error may be ignored by a
developer
who is using the API in a wrong way, where as the second will hopefully make him
read the docs and realize his mistake.
Because of this I'm in favor with keeping my somewhat more complex version.
2) You change from switch cases to str tables.
I can understand that this is a tempting solution, but I don't like the tricks
you play wrt the other and unknown errors, the need for these tricks to me
shows that this is not the best approach.
> "and it'll actually break compilation if people add a new message and forget
> to add a string for the languages"
No unfortunately it does not, having less initializers then array elements is
absolutely
allowed by the C spec, the rest of the array will simply be zero filled (with
gcc even
having excess initializer elements is only a warning, with the extra ones
ignored).
This together with your tricks for other and unknown means that the newly added
but
untranslated error will get the string for LIBUSB_ERROR_OTHER,
LIBUSB_ERROR_OTHER will
get the string for unknown errors, and unknown errors will return NULL.
One of the big advantages of having switch cases is that they give a direct
mapping
between errors and strings, thus things like this example, where error strings
shift
to a different error code if an initializer is missing won't happen. This will
also
make it lot easier for translators to see which string belongs to which error
code,
instead of them needing to count the number of prior initializers to get the
integer
value of the error code, and then look up that integer value in the enum, by
counting
backwards ...
Another advantage of my code is that it will fallback to English error strings
when
their is no translated one, but your code can be easily modified to do the same
by
inserting NULL entries into the array.
Given the above I'm strongly in favor of keeping a switch case here.
Regards,
Hans
------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. A cloud service to automate IT design, transition and operations
2. Dashboards that offer high-level views of enterprise services
3. A single system of record for all IT processes
http://p.sf.net/sfu/servicenow-d2d-j
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
