On 19/09/13 22:35, Pete Batard wrote:
> OK, a few preliminary remarks:
>
> 1.
>
> TCHAR: No. Just no. Please get rid of this atrocity. Either you use
> an UTF-8 char* string (or you use a wchar_t if you must need
> UTF-16). Your use of TCHAR completely breaks VS2012 compilation
> right now and despite what Microsoft wants to make everybody
> believe, there is no justification for using TCHAR ever (except for
> letting Redmond get away with not doing what they should have done
> about 15 years ago <http://www.utf8everywhere.org/>)\
Yeah I don't like TCHAR either, but as msdn was using it, I had no idea
what to replace it with, and was kind of lazy.
> 2.
>
> Be mindful that
>
> |int a;
> a = 1;
> int b;
> b = a+1;
> |
>
> Will not work on MS compilers. All the variable declarations must be
> at the top.
> Thus windows_usb.c lines 584 & 2366 also break MSVC compilation.
Really? Is that stupid?
> 3.
>
> b9eae35
>
> <https://github.com/libusbx/libusbx/commit/b9eae35836855367a0e779c349ae333db8564a3b>
> => nope, not gonna work. MSVC doesn't have named initializers (and
> that's not limited to MSVC6). You need to revert this whole patch.
Ok no problem, someone from inside the company noticed the same, I've
been using mingw and cygwin only.
>
> 4.
>
> When the *existing* code uses something like
>
> |int func(int a, int b, int c);
> |
>
> It's very bad manner to disregard the previous styling convention
> and introduce your new code with
>
> |int newfunc(int d,
> int e,
> int f)
> |
>
> Whoever was first to create a source gets to decide the styling
> convention, and everyone who comes after follows. Now, if you create
> a new sourcefile, you get to pick whatever you like (as long as it's
> not too wild), but if you add to an existing one, you just spend
> some time looking around at the existing style, and follow it.
> Same goes for using spaces in code addons where spaces are used. I
> see quite a few of these.
> Please use an editor that displays whitespace symbols and address
> your spaces vs tabs issues. Also, a proper git UI (such as
> TortoiseGit on Windows) is very efficient at letting you know when
> you're introducing whitespace issues and it's as annoying for a
> maintainer as it is for a submitter to see one's patch shot down
> simply because they didn't bother paying attention to whitespace issues.
Yes sorry my fault, it's just that I'm using linux for development with
emacs and those lines are just too long to actually be readable (yes I'm
doing 80 chars here!). I got used to some linux kernel development long
time ago and now can't leave it behind as I really like it. But I can
fix that no problem.
I did paid attention to the tabs/whitespace nightmare in libusb, what
did I miss? Did I introduce any spaces where there should be tabs? I'll
review my changes again.
BTW shouldn't there be just one code guideline for the hole project?
> 5.
>
> f3dd52c
>
> <https://github.com/libusbx/libusbx/commit/f3dd52c02212c62e154972093d832f1b83f794f1>
> - I'm not sure how I feel about having emacs defs introduced to
> Windows specific files that *most* people are going to edit on
> Windows, with editors that aren't emacs and that have their styling
> sensibly set *globally* without a requirement to have it def'd
> within the file itself (ugh! Besides vi > emacs anyway ;)). Can you
> imagine if every editor out there insisted on doing the same? We'd
> end up with a whole set of /* Joe's editor // .... // Bob's editor
> // ... // Bill's editor */ sections that'd end up taking more space
> than the actual payload of the file. Now, if you *must* have it, and
> since OS X has already started doing some of that, I'll let it
> slide, but really, who in their right mind can justify that merging
> and multiplying editor specific presentation config data in each so
> urce of a project is a good idea.
Ok I will look if there's any way I can set this up without interfering
with my global settings. My setup is linux+emacs with a vm running win7 64b.
> 6.
>
> If you don't have a Windows machine at hand, *strongly* suggest that
> you install Wine and the latest *standalone* WDK (I think this
> <http://www.microsoft.com/en-us/download/details.aspx?id=11800> link
> should do), so that you can pick up on all the annoyances that the
> MS compiler is going to throw at you. If you have a Windows test
> machine (which I guess you probably do, since I don't see why you'd
> want to spend time on a Windows hp implementation in the first
> place), then it's even easier - just install the WDK, and run
> ddk_build.cmd in the msvc directory. By the way the WDK is free and
> is as close as you can get to MSVC if you can't afford the hefty
> Visual Studio license.
> It's also a good idea to test both cygwin & MinGW compilation if you
> can. Basically, the first thing I'm going to do is check whatever
> errors and warnings come out out of MSVC (either WDK or VS2012),
> MinGW (one of 32 or 64 depending on how the stars are aligned) and
> cygwin. If either one of those doesn't compile cleanly, I'll ask you
> to make it so (and yeah, this kind of multiplies your amount of
> testing by 3 - welcome to not time consuming at all libusb Windows
> backend development!).
I will go ahead and test with WDK, I have it somewhere. I think I may
have MSVC somewhere. MinGW does build and run as that's my main
compiler, only I'm using SCons for building the project, I tried with
cygwin as well, and it was working long time ago.
>
> If you try to address the above, I'll try to get a more serious look at
> your actual implementation (though it may be a couple more weeks before
> I actually do so).
Sure I can do that, sometime next week I think.
Let me refactor the code and I send a more clean code.
-Manuel
---
Reply to this email directly or view it on GitHub:
https://github.com/libusbx/libusbx/issues/9#issuecomment-24785995
------------------------------------------------------------------------------
LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!
1,500+ hours of tutorials including VisualStudio 2012, Windows 8, SharePoint
2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack includes
Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/20/13.
http://pubads.g.doubleclick.net/gampad/clk?id=58041151&iu=/4140/ostg.clktrk
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
