Thanks Manuel.
* b8d13f2d0d40ffa3addd38f12b0152d942135661: Please don't try to squeeze your
styling when I specifically warned you about it. There are way too many styling
issues with this commit: I'm seeing double spaces converted to tabs in printf's
that are unrelated to your patch, braces at the end of an if statement that are
missing the leading space, and I also said not to split,
mutilple,
arguments,
over,
multiple,
lines.
Sorry but, while I'm still willing to believe these are honest mistakes, I
can't let style slide. Multistlyled code is a pain for everybody, not just the
maintainers. Unless you created a new source from scratch, you just pay
attention to whatever existing style was used and follow it, starting with the
very first commit you want us to integrate. I already have 19 lines to fix
[just for the styling of this one
patch](https://github.com/pbatard/libusbx-hp/commit/96b0255c2aab77d82a09717b568c3524e92377ae),
and I don't really want to have to spend time fixing styling issues.
* b3f0ac528457e9da19039aa8c5cf855002a93dbd: would have to track why we did this
in the first place, which I'm not gonna do, so no objection on removing this
cast and letting time tell if it was actually needed in the first place.
* 7b4eaa9855f307465c29649564af5157a7a12d7e: I don't really see anything that
needs fixing there. We're setting `usbi_default_context` to NULL but ctx is
still perfectly valid. Plus you're modifying `usbi_default_context` outside of
its mutex lock (not a good idea), and what's more, splitting the message that
says we're destroying the default context (which we're not really doing here)
from the part where we're removing the user reference. Unless you've
experienced a real issue there, which we'll need to analyse better, this commit
is NO_GO.
* b86964ace5f2f9c088d56b17c56ffa603ef5d9f3: Don't really care, if Toby is also
OK with this.
* 17d05f567c04868406ca6402b1a5d378d687a171: No real issue with that either. I'd
prefer to see inline helper calls towards the top of a source rather than added
at the bottom, but that's minor and I'm not going to force you to redo that one.
* 3de7d1d9cb4299fc9cfd10abcd16416289a5640c: If we still need to have direct
reference to the abomination that is a T-string in our code, then something is
very wrong. And indeed something still is. Microsoft wasn't as foolish as have
any of their __A__ calls take a `PCTSTR`/`PTSTR`, and your def should be:
```
LL_DECLARE_PREFIXED(WINAPI, HDEVINFO, p, SetupDiGetClassDevsExA, (const GUID*,
PCSTR, HWND, DWORD, HDEVINFO, PCSTR, PVOID));
DLL_DECLARE_PREFIXED(WINAPI, BOOL, p, SetupDiGetDeviceInstanceIdA, (HDEVINFO,
PSP_DEVINFO_DATA, PSTR, DWORD, PDWORD));
```
While we're on the subject, the informal rule I've tried to follow is: for
anything that returns strings that we're pretty sure is never going to see any
localized character (GUID, Device Interface Path, etc.), use an explicit __A__
call. For items that may see a localized string (eg. directory path) use the
__W__ call and convert the string to UTF-8 if we need to feed it to other parts
of the library. Or, to put it more simply, just consider that everything is
UTF-8 everywhere, as long as Microsoft doesn't provide us with any other choice
but to use __W__.
That's not to say I haven't missed a few calls where __A__ should have been
explicitly specified explicitly in `windows_usb.c`. If that's the case, we'll
need to fix that as well.
* 327aac878e127a7b2ec7b82977bbd7dea05b5133: I'll review the bulk of the hotplug
patch when I have more time in front of me
* a3f315d4b35ba34955a1ecf9900032b92e8f5578: We have a lowercase `bool` and
that's what we (try to) use everywhere where Microsoft doesn't forces us
otherwise. Please use that to preserve styling. You also have,
frigging,
parameters,
on,
separate,
lines, in this commit and the previous one. I'm going to be a pain, but not
having to spend half a comment complaining about styling inconsistencies is
what I'd prefer, so it would be nice if these were to be eliminated.
* dfa1d55947ae6cc3433de5b610f849993fd7fb79: You're gonna have to merge that
with 327aac878e127a7b2ec7b82977bbd7dea05b5133. `TCHAR` is not OK ever, even
temporarily.
* c94f9d5dbe74473938e66c8ac5fa847eb2c960b2: Likewise, would be better merged
with the earlier patches.
I hope I'm not sounding too harsh, because I do appreciate the work you put in
so far, and if it achieves what we want, I very much want to see your patch
integrated one way or the other.
Still, I'd rather get all the styling and grouping/merging issues out of the
way, to ensure that we can review what actually matters, as well as have
commits that could actually be integrated as is, if we think the code behind
them is sound.
/Pete
---
Reply to this email directly or view it on GitHub:
https://github.com/libusbx/libusbx/issues/9#issuecomment-26013240
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
