Re: [PATCH] Add native Bluetooth support for Windows platforms
On Wednesday 19 August 2015 00:57:20 Claudiu Olteanu wrote: > I tried as well to replace the WSAStringToAddressA with > WSAStringToAddressW but I had to represent the > address as a wchar_t* and and when I wanted to use > mbstowcs_s for conversion (from const char* to wchar_t*) > the compiler couldn't find the declaration to the method > (even though I included the stdlib header). After some > failed attempts I gave up :). Hi Claudiu I will review the patches again tonight. Please don't use anything from like mbstowcs or whatever. Just use QString::fromUtf16 and toLocal8Bit or toLatin1, depending on context. I will take a look at the code when I review and suggest how to do it. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATCH] Add native Bluetooth support for Windows platforms
On Wednesday 19 August 2015 00:57:20 Claudiu Olteanu wrote: > Hi there, > > As I promised, I created a new set of patches. You can find it > attached to this e-mail. > > Here is a list with the changes I did after the feedback: > - initialized the internal variables (patch 06) Thanks! > - moved the *WSACleanup* call from patch 06 to patch 12 Good, like I said that is now better than Qt itself. > - removed the implementation of my internal *getLastError* and started > using *qt_error_string* (patch 11) > - used *WSAAddressToStringW* instead of *WSAAddressToStringA* (patch 11) > - avoided some code duplication (patches 14, 15, 17) > > I hope that I covered all of Thiago's suggestions. Yes, it does. > Also I didn't remove the call of strdup method from > *qtserialbluetooth::qt_serial_open *because the > *WSAStringToAddressA* method is expecting to receive > LPSTR {aka char*} parameter while I have the address > represented as const char*. The Win32 API documentation says it's an input parameter. It's probably a mistake on MSFT's part to have forgotten the "const" there. But let's not tempt fate and we'll leave it with the strdup. > I tried as well to replace the WSAStringToAddressA with > WSAStringToAddressW but I had to represent the > address as a wchar_t* and and when I wanted to use > mbstowcs_s for conversion (from const char* to wchar_t*) > the compiler couldn't find the declaration to the method > (even though I included the stdlib header). After some > failed attempts I gave up :). Like I said, never, ever use functions. Those are braindead in design, at least compared to Qt's equivalents. Before I go into this, let me say that the patches are fine and Dirk can apply them as-is. There's no need to change further. With that now said, here's how you can improve. Your code is: > + char *address = strdup(devaddr); [...] > + if (WSAStringToAddressA(address, > + AF_BTH, > + NULL, > + (LPSOCKADDR) &socketBthAddress, > + &socketBthAddressBth > + ) != 0) { > + To use the W function: QString address = QString::fromLatin1(devaddr); if (WSAStringToAddressW(reinterpret_cast(address.utf16()), [...] This also solves the strdup() and free() calls. And if the line didn't get too long, you could do everything in one line. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATCH] Add native Bluetooth support for Windows platforms
hello, chiming in for a couple of comments. On 21 August 2015 at 07:22, Thiago Macieira wrote: > On Wednesday 19 August 2015 00:57:20 Claudiu Olteanu wrote: > >> I tried as well to replace the WSAStringToAddressA with >> WSAStringToAddressW but I had to represent the >> address as a wchar_t* and and when I wanted to use >> mbstowcs_s for conversion (from const char* to wchar_t*) >> the compiler couldn't find the declaration to the method >> (even though I included the stdlib header). After some >> failed attempts I gave up :). > Like I said, never, ever use functions. Those are braindead in > design, at least compared to Qt's equivalents. > > Before I go into this, let me say that the patches are fine and Dirk can apply > them as-is. There's no need to change further. Mingw doesn't have an exact port of all of the _s functions in MSVC - some of them are present in stdlib_s.h, including mbstowcs_s. i see it the following way - the "W" variants is only there for compatibility with the rest of the WIN32. the address itself *should be* ASCII-US, therefore the "W" API is not needed here and WSAStringToAddressA/WSAStringToAddressA can be used instead (?). > > > With that now said, here's how you can improve. Your code is: > >> + char *address = strdup(devaddr); > [...] >> + if (WSAStringToAddressA(address, >> + AF_BTH, >> + NULL, >> + (LPSOCKADDR) &socketBthAddress, >> + &socketBthAddressBth >> + ) != 0) { >> + > > To use the W function: > > QString address = QString::fromLatin1(devaddr); > if (WSAStringToAddressW(reinterpret_cast(address.utf16()), > [...] > > This also solves the strdup() and free() calls. And if the line didn't get too > long, you could do everything in one line. > i don't think WSAStringToAddressA will modify the input string unless explicitly stated in the documentation. the WIN32 API seems to be consistent in terms of input/output ownership - i.e. "i will only modify the output targets you give me and not touch your input targets". i'm pretty sure this should work: WSAStringToAddressA((char *)devaddr... lubomir -- ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATCH] Add native Bluetooth support for Windows platforms
Sorry for the late response but I didn't have access to Internet for the last three days. Thiago, thanks again for the review. I saw that the patches are now in the upstream. Dirk, if you think that it is necessary I can send you another patch using the WSAStringToAddressW (like Thiago suggested), or forcing the cast from const char* to char * (like Lubomir suggested) and continue to use the WSAStringAddressA function. Have a nice day, Claudiu ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATCH] Add native Bluetooth support for Windows platforms
On Mon, Aug 24, 2015 at 11:20:29AM +0300, Claudiu Olteanu wrote: > Sorry for the late response but I didn't have access to Internet for the > last three days. > > Thiago, thanks again for the review. I saw that the patches are now in > the upstream. > > Dirk, if you think that it is necessary I can send you another patch using > the WSAStringToAddressW (like Thiago suggested), or forcing the cast > from const char* to char * (like Lubomir suggested) and continue to use the > WSAStringAddressA function. Obviously this is past the GSoC end, so we are now moving from "you are a GSoC student" to "you are one of our top contributors". Which means this is your decision :-) I think I lean slightly towards the WSAStringToAddressW solution as it seems a bit more elegant (if anything in the Windows API is ever elegant). /D ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface