Hi,
On 06/08/2013 04:56 PM, Pete Batard wrote:
> On 2013.06.08 13:05, Hans de Goede wrote:
<snip>
>> 1) You simplify the setlocale function
>
> Well, duh.
>
> I think I've been pretty clear that I've been trying to spend the least
> amount of time I can looking at this localization charade.
> But if you want to add a full blown ISO 639-1-plus validator in
> setlocale ("plus", since we're not going for strict ISO 639-1), I'm not
> gonna prevent you.
>
> Basically, it only takes checking for the locale string being exactly 2
> chars long or having a dash in 3rd to return INVALID_PARAM.
a dash or a dot, ie accept "nl.UTF-8".
<snip>
> To be honest, I think you are stretching this out a little bit. If a
> developer receives an error from a call, they should try to investigate
> it. The scenario you describe really boils down to a developer not
> willing to read the doc (to find out the locale should start with an ISO
> 639-1 code) and trying things at random whilst also not checking for
> error codes... But then again, as pointed above, modifying my proposal
> to return INVALID_PARAM if someone tries to feed "Dutch" is very
> straightforward.
Ok, so lets modify your proposal with an INVALAD_PARAM check then, and
move forward with your proposal.
>
>> 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,
>
> Short of time. You want a less "tricky" approach, sure:
>
> static int usbi_errcode_to_message_index(enum libusb_error errcode) {
> switch(errcode) {
> case LIBUSB_SUCCESS:
> return 0;
> case LIBUSB_ERROR_IO:
> return 1;
> ...
> case LIBUSB_ERROR_OTHER:
> default:
> return LIBUSB_ERROR_COUNT-1;
> }
> }
>
> and use that elsewhere in the code where we want the index. Just adds
> more boilerplate.
I agree this is more boiler plate, my main concern with using a simply
lookup array is not the special handling of these 2 cases (the special
handling is not pretty, but if we go for a lookup array I see no
better way).
My main concern is all the strings ending up shifted because of a
missing entry in the initializer array.
<snip>
> Then again, even without this, it's no worse than your solution, that
> also cannot tell whether a localized message for an error code is missing.
Well it is worse in the sense that in my code it will fall back to the
English message for that error code, where as with your code we end
up with the wrong string or possible even NULL.
As said this is my main concern, a concern which you address here:
> That sure will be a bummer for libusbx internal developers (not
> libusbx-based app developers) who just introduced a new error code,
> ignored the very explicit part right at the place they added the code
> about adding new messages in strerror.c, and didn't test their code at
> all... How many people does that include?
>
> The one thing I hope I can make you realize here is that there's a
> fundamental difference between trying to add extra safeguards for
> regular libusbx users, which is usually something we want to spend time
> on, and adding extra safeguards for libusbx developers (us), which we
> may very well try to avoid if the problems those safeguards would
> prevent can and will easily be spotted.
A valid point, something which I've been thinking about too, so lets
go with the lookup table (iow your) approach, and hope my worries never
turn into reality.
tl;dr: Please add a sanity check on locale, ie, check that
(strlen(locale) == 2 ||
(strlen(locale) > 2 &&
(locale[2] == '_' || locale[2] == '.')))
is true, and otherwise return an invalid_param error, and then
lets go with your proposal.
Regards,
Hans
p.s.
Note the strlen check in my proposed sanity check, some calling code
may very well end up passing in just "C", so the strlen check is
important!
------------------------------------------------------------------------------
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
