On 2013.06.08 13:05, Hans de Goede wrote:
> Not working because of the project file issues, or did you find other
> issues too ?
MSVC was not working because strerror.c was not referenced.
MinGW was not working because it is referencing the dll by default, and
the .def file used to generate the import library was wrong => symbol
not found.
I'm not aware of other issues, but I didn't test your code besides
compilation.
> 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.
> 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.
Which would still make it EXCEEDINGLY OBVIOUS for any app developer to
find out that they passed a wrong parameter. Also, libusb(x) has a
history (which I also tried to fight at the time) of not doing extra
validation on parameters that it can avoid.
> 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.
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.
> 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.
> the need for these tricks to me
> shows that this is not the best approach.
As pointed out above, I'm not using "tricks" because I have to, but
because I didn't want to waste more time than needed on this, and I fail
to see the part where there's something fundamentally tricky about
converting something into a bounded array index to reference array data.
Wanting to separate locale data from the code, so that translators, who
may or may not be familiar with C, are less prone to introducing
problems is just best practice really. Plenty of applications do
conversion from enum or whatever to index in order to use string arrays,
and it helps keep things clearly defined and easier to maintain. That's
not a "trick".
> > "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,
Yeah, you're right there. I forget that it only works in one direction
(too many initializers) and not the other.
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.
> 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.
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.
App developers won't add new error codes in libusb.h. We will. And when
we do, I'm confident that we will pay heed to the message that tells us to:
1. increase LIBUSB_ERROR_COUNT
2. add the extra messages in the tables (even if it's only a "THIS NEW
ERROR CODE NEEDS TRANSLATION").
Now, if you're trying to pretend we need to go the extra mile for the 10
people tops that will ever add new error codes, and that are supposed to
be familiar enough with how libusbx works and how it implements
localization, be my guest. But don't be surprised if I don't feel like
jumping on that boat.
<snip> the part about the shifting of error codes, since it boils down
to the same as above.
> Given the above I'm strongly in favor of keeping a switch case here.
I don't have a problem with having a switch, as long as there's only
one, and it'd used for enum -> array index conversion. Anything else
sounds way more complex than it should be, and more prone to get
problems introduced, which we will actually have a harder time spotting.
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
