Hi,
On 06/09/2013 02:28 AM, Pete Batard wrote:
> On 2013.06.08 19:22, Hans de Goede wrote:
>> a dash or a dot, ie accept "nl.UTF-8".
>
> Good point.
>
>> Ok, so lets modify your proposal with an INVALAD_PARAM check then, and
>> move forward with your proposal.
>
> OK. If I understand the rest of your message properly, you don't want the
> function with the switch for the errcode to index conversion, right?
Right.
> Coz if we go with that, we will then need to update 3 things when adding a
> new error code:
> - LIBUSB_ERROR_COUNT
> - New message strings for each locale
> - A new case for the errcode to index switch
>
> While the first two are hard to miss IMO, the 3rd, which would only come from
> that extra function call, not as much. Also, I think the whole switch just to
> return mostly increasing integers would look kind of silly,
Agreed that using a switch for this is silly :)
>> My main concern is all the strings ending up shifted because of a
>> missing entry in the initializer array.
>
> Understood. As you pointed out, I don't see much of a concern with this,
> since we'are be the only ones susceptible to break libusbx in such a fashion,
> and we're supposed to be paying attention to our own very prominent comments
> when adding an errcode.
>
>> 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.
>
> As per the attached, here's what I'm planning to go with:
>
> if ( (locale == NULL) || (strlen(locale) < 2)
> || ((strlen(locale) > 2) && (locale[2] != '-') && (locale[2] != '_') &&
> (locale[2] != '.')) )
> return LIBUSB_ERROR_INVALID_PARAM;
>
> I added a dash in there, since Windows culture codes are of the form "nl-NL",
> "fr-BE", etc, and we should support these. Not sure if your underscore was
> actually intended, but even if it wasn't, I see no real harm having it.
> I tested this check with xusb and it seemed to produce the expected results.
The _ was deliberate as that is what Linux (POSIX in general I guess) uses, the
proposed sanity check looks good.
>> 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!
>
> Absolutely. Strings provided by a user can never be trusted in terms of
> length or their data being non-garbage.
>
> Finally, as you'll see from the new proposal, I also added a doxygen comment
> to detail the steps translators should follow to add a translation.
Good idea!
New proposal looks good, ack!
Only one small nitpick which could be fixed before committing it to master:
+ if (i >= ARRAYSIZE(usbi_locale_supported)) {
+ usbi_warn(NULL, "Unrecognized locale format: %s", locale);
+ return LIBUSB_ERROR_NOT_FOUND;
+ }
I wonder if we should warn here at all? With gettext if there is no translation
programs silently fall back to english, we could do the same. Either way (with
or without warning) is fine with me, just wondering whats best.
If we keep the warning I would make the warning:
usbi_warn(NULL, "Unrecognized language in locale: %s", locale);
Since we've already varied the locale string format at this point.
Regards,
Hans
>
> Regards,
>
> /Pete
>
>
> ------------------------------------------------------------------------------
> 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
>
------------------------------------------------------------------------------
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
