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
