August 2016 - Wine-Devel - list.winehq.org

Re: [v2 4/6] msvcirt: Implement istream::operator>> for integers.

by Iván Matellanes

On 02/08/16 14:32, Piotr Caban wrote: > On 08/02/16 13:37, Iván Matellanes wrote: >> +static istream* istream_internal_read_unsigned_integer(istream *this, >> void *p, int size) >> +{ >> + /* size: 0 => short, 1 => int, 2 => long */ >> + ios *base = istream_get_ios(this); >> + char buffer[16]; >> + int num_base; >> + LONG min_value[3] = {SHRT_MIN, INT_MIN, LONG_MIN}; >> + ULONG umax_value[3] = {USHRT_MAX, UINT_MAX, ULONG_MAX}, tmp; > Shouldn't min_value be equal to 0? These operators allow extracting negative values into unsigned variables, after performing the appropriate casting, e.g.: {type_uint, "-1", ..., /* 4294967295 */ 0, IOSTATE_eofbit, ...}, {type_uint, "-2", ..., /* 4294967294 */ 1, IOSTATE_eofbit, ...}, {type_uint, "-2147483648", ..., /* 2147483648 */ 2, IOSTATE_eofbit, ...} > > How about changing the helper functions to have following prototypes: > static LONG istream_internal_read_integer(istream *this, LONG min_value, > LONG max_value); > and > static ULONG istream_internal_read_unsigned_integer(istream *this, ULONG > max_value); Yes, it looks better this way. Same for the floats helper. Thanks, Iván.

Aug. 2, 2016

Re: [v2 5/6] msvcirt: Implement istream::operator>> for floats.

by Piotr Caban

On 08/02/16 13:37, Iván Matellanes wrote: > +static istream* istream_internal_read_float(istream *this, void *p, int size) > +{ > + /* size: 0 => float, 1 => double, 2 => long double */ > + const int max_chars[3] = {20, 28, 32}; /* character count is limited on Windows */ > + char buffer[32]; > + double d; > + > + TRACE("(%p %p %d)\n", this, p, size); > + > + if (istream_ipfx(this, 0)) { > + if (istream_getdouble(this, buffer, max_chars[size]) > 0) { > + d = strtod(buffer, NULL); > + if (size == 0) { > + /* check whether the value fits in the output var */ > + if (d > FLT_MAX) > + d = FLT_MAX; > + else if (d < -FLT_MAX) > + d = -FLT_MAX; > + else if (d > 0 && d < FLT_MIN) > + d = FLT_MIN; > + else if (d < 0 && d > -FLT_MIN) > + d = -FLT_MIN; > + *((float*) p) = d; I think that this helper will be nicer if it has following prototype: static double istream_internal_read_float(istream *this, int max_chars); where max_chars is set to 20, 28 or 32 depending if float, double or LDOUBLE needs to be read. With FLT_MIN/FLT_MAX checks moved to operator>>(float) implementation. Thanks, Piotr

Aug. 2, 2016

Re: [v2 4/6] msvcirt: Implement istream::operator>> for integers.

by Piotr Caban

On 08/02/16 13:37, Iván Matellanes wrote: > +static istream* istream_internal_read_unsigned_integer(istream *this, void *p, int size) > +{ > + /* size: 0 => short, 1 => int, 2 => long */ > + ios *base = istream_get_ios(this); > + char buffer[16]; > + int num_base; > + LONG min_value[3] = {SHRT_MIN, INT_MIN, LONG_MIN}; > + ULONG umax_value[3] = {USHRT_MAX, UINT_MAX, ULONG_MAX}, tmp; Shouldn't min_value be equal to 0? How about changing the helper functions to have following prototypes: static LONG istream_internal_read_integer(istream *this, LONG min_value, LONG max_value); and static ULONG istream_internal_read_unsigned_integer(istream *this, ULONG max_value); I think the code will be nicer this way. Thanks, Piotr

Aug. 2, 2016

Re: [v2 3/6] msvcirt: Implement istream::operator>> for strings.

by Piotr Caban

On 08/02/16 13:37, Iván Matellanes wrote: > @@ -3514,7 +3514,28 @@ istream* __thiscall istream_read_char(istream *this, char *ch) > DEFINE_THISCALL_WRAPPER(istream_read_str, 8) > istream* __thiscall istream_read_str(istream *this, char *str) > { > - FIXME("(%p %p) stub\n", this, str); > + ios *base = istream_get_ios(this); > + int ch, count = 0; > + > + TRACE("(%p %p)\n", this, str); > + > + if (istream_ipfx(this, 0)) { > + for (ch = streambuf_sgetc(base->sb); > + str && count < (unsigned int) base->width - 1 && !isspace(ch); > + ch = streambuf_snextc(base->sb)) { streambuf_sgetc shouldn't be called if str == NULL. Thanks, Piotr

Aug. 2, 2016

Re: [PATCH 2/5] mpr: Implement provider selection given remote name.

by Nikolay Sivov

On 02.08.2016 15:17, Pierre Schweitzer wrote: > Le 02/08/2016 à 14:13, Nikolay Sivov a écrit : >> On 02.08.2016 14:31, Pierre Schweitzer wrote: >>> + len = WideCharToMultiByte(CP_ACP, 0, provider->lpProvider, -1, NULL, 0, NULL, NULL); >>> + provider_name = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); >>> + if (provider_name) >>> + memcpy(provider_name, provider->lpProvider, len * sizeof(WCHAR)); >>> + >>> + HeapFree(GetProcessHeap(), 0, provider); >>> + >>> + return provider_name; >> >> This is confusing. WNetGetResourceInformationW return W string, you >> convert it to A, and use A length to allocate another W buffer? What's >> going on? > > I'm just getting string length for a local recopy, as it is done in > other places. > Should I use something else? Like some strdupW if it exists? If you already have W string, strlenW will give you length. What are the other places where it's done like that?

Aug. 2, 2016

Re: [PATCH 2/5] mpr: Implement provider selection given remote name.

by Pierre Schweitzer

Le 02/08/2016 à 14:13, Nikolay Sivov a écrit : > On 02.08.2016 14:31, Pierre Schweitzer wrote: >> + len = WideCharToMultiByte(CP_ACP, 0, provider->lpProvider, -1, NULL, 0, NULL, NULL); >> + provider_name = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); >> + if (provider_name) >> + memcpy(provider_name, provider->lpProvider, len * sizeof(WCHAR)); >> + >> + HeapFree(GetProcessHeap(), 0, provider); >> + >> + return provider_name; > > This is confusing. WNetGetResourceInformationW return W string, you > convert it to A, and use A length to allocate another W buffer? What's > going on? I'm just getting string length for a local recopy, as it is done in other places. Should I use something else? Like some strdupW if it exists? -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

Aug. 2, 2016

Re: [PATCH 4/5] mpr: Implement WNetCancelConnectionA(), WNetCancelConnectionW(), WNetCancelConnection2A().

by Nikolay Sivov

This looks okay to me.

Aug. 2, 2016

Re: [PATCH 1/5] mpr: Implement local name automatic redirection for disks. (try 3)

by Nikolay Sivov

On 02.08.2016 14:31, Pierre Schweitzer wrote: > + FIXME("Locale device selection is not implemented for printers.\n"); > + return WN_NO_NETWORK; This should be 'local', and not 'locale'.

Aug. 2, 2016

Re: [PATCH 2/5] mpr: Implement provider selection given remote name.

by Nikolay Sivov

On 02.08.2016 14:31, Pierre Schweitzer wrote: > + len = WideCharToMultiByte(CP_ACP, 0, provider->lpProvider, -1, NULL, 0, NULL, NULL); > + provider_name = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); > + if (provider_name) > + memcpy(provider_name, provider->lpProvider, len * sizeof(WCHAR)); > + > + HeapFree(GetProcessHeap(), 0, provider); > + > + return provider_name; This is confusing. WNetGetResourceInformationW return W string, you convert it to A, and use A length to allocate another W buffer? What's going on?

Aug. 2, 2016

Re: [PATCH 1/5] mpr: Implement local name automatic redirection for disks. (try 3)

by Pierre Schweitzer

Le 02/08/2016 à 14:06, Nikolay Sivov a écrit : > On 02.08.2016 14:31, Pierre Schweitzer wrote: >> This fixes a bug seen during review for ReactOS commit: in case of local >> redirection, the name wasn't properly converted back to ANSI for A callers. >> >> > > >> -static void use_connection_set_accessnameW(struct use_connection_context *ctxt) >> +static void use_connection_set_accessnameW(struct use_connection_context *ctxt, WCHAR *local_name) >> { >> WCHAR *accessname = ctxt->accessname; >> - if (ctxt->resource->lpLocalName) >> - strcpyW(accessname, ctxt->resource->lpLocalName); >> + if (local_name) >> + strcpyW(accessname, local_name); >> else >> strcpyW(accessname, ctxt->resource->lpRemoteName); >> } > > You already have lpLocalName here, why another argument? Because lpLocalName isn't necessarily set to the correct value in case of automatic redirection. It was asked by A. Julliard: not to modify context resource. -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

Aug. 2, 2016