From: Nikolay Sivov <nsivov@codeweavers.com>
Subject: Re: [PATCH] find: First simple implementation and tests
Message-Id: <8e7fe51f-d844-1519-183b-56d357cf9bd5@codeweavers.com>
Date: Sun, 16 Sep 2018 16:13:51 +0300
In-Reply-To: <9669248.GM1dufnK2Z@arch>
References: <20180915154246.8132-1-dark.shadow4@web.de> <58776d26-8f5d-9871-fd50-63826c053cfe@codeweavers.com> <9669248.GM1dufnK2Z@arch>

On 09/15/2018 11:38 PM, Fabian Maurer wrote:

> > > + /* Read next content into buffer */
>
> > > + if (buffer_pos >= buffer_max)
>
> > > + {
>
> > > + BOOL success = ReadFile(handle, buffer, 4096, &buffer_max, NULL);
>
> > > + if (!success)
>
> >
>
> > Separate variable does not seem to be necessary.
>
> BOOL success you mean? I always pull that out of an if to make it more 
> readable.
>
> > > + line_converted_length = MultiByteToWideChar(CP_ACP, 0, line, -1, 0,
>
> > > 0); + line_converted = heap_alloc(line_converted_length *
>
> > > sizeof(WCHAR)); + MultiByteToWideChar(CP_ACP, 0, line, -1,
>
> > > line_converted, line_converted_length); +
>
> > > + heap_free(line);
>
> > > +
>
> > > + *line_out = line_converted;
>
> > > + return TRUE;
>
> > > +}
>
> >
>
> > CP_ACP is probably not correct in this case.
>
> Maybe, but it works in the tests for now. What would you propose?
>

Again, take a look how output works in reg or regedit.

> >
>
> > > +void write_to_handle(HANDLE handle, const WCHAR *str)
>
> > > +{
>
> > > + DWORD bytes_written_sum = 0;
>
> > > + char *str_converted;
>
> > > + UINT str_converted_length;
>
> > > + UINT str_length = lstrlenW(str);
>
> > > +
>
> > > + str_converted_length = WideCharToMultiByte(CP_ACP, 0, str,
>
> > > str_length, NULL, 0, NULL, NULL); + str_converted =
>
> > > heap_alloc(str_converted_length);
>
> > > + WideCharToMultiByte(CP_ACP, 0, str, str_length, str_converted,
>
> > > str_converted_length, NULL, NULL); +
>
> > > + do
>
> > > + {
>
> > > + DWORD bytes_written;
>
> > > + WriteFile(handle, str_converted, str_converted_length,
>
> > > &bytes_written, NULL); + bytes_written_sum += bytes_written;
>
> > > + } while (bytes_written_sum < str_converted_length);
>
> > > +
>
> > > + heap_free(str_converted);
>
> > > +}
>
> > > +
>
> > > +void find_printf(const WCHAR *str)
>
> > > +{
>
> > > + write_to_handle(GetStdHandle(STD_OUTPUT_HANDLE), str);
>
> > > +}
>
> >
>
> > Check how it's done in reg/regedit, probably you should reuse it here.
>
> > Less important, but why two helpers here? And why do you need a loop to
>
> > write this buffer?
>
> I'll take a look at it. AFAIK WriteFile doesn't guarantee all bytes to 
> be written at once, that's why we have the "bytes written" parameter. 
> Correct me if I'm wrong.
>

I think it's guaranteed for synchronous case.

> > There's strstrW(), importing shlwapi only for that is too much.
>
> Ah, missed that one. I'll change it.
>
> >
>
> > > + WCHAR message_parameter_invalid[64];
>
> > > + WCHAR message_switch_invalid[64];
>
> > >
>
> > > int i;
>
> > >
>
> > > + LoadStringW(GetModuleHandleW(NULL), IDS_INVALID_PARAMETER,
>
> > > message_parameter_invalid, sizeof(message_parameter_invalid)); +
>
> > > LoadStringW(GetModuleHandleW(NULL), IDS_INVALID_SWITCH,
>
> > > message_switch_invalid, sizeof(message_switch_invalid));
>
> > You don't need them both at the same time. Also check the arguments.
>
> Sure thing, just loaded all of them at once for simplicity. 
> Performance wise it shouldn't matter, but do you want me to load them 
> only when needed?
>
> By checking the arguments, do you mean I should call it with buffer 
> length 0 to get a readonly pointer?
>

I mean length is wrong.

> >
>
> > > + input = GetStdHandle(STD_INPUT_HANDLE);
>
> > > +
>
> > > + for (i = 1; i < argc; i++)
>
> > > + {
>
> > > + if (argv[i][0] == '/')
>
> > > + {
>
> > > + find_printf(message_switch_invalid);
>
> > > + return 2;
>
> > > + }
>
> > > + else if(tofind == NULL)
>
> > > + {
>
> > > + tofind = argv[i];
>
> > > + }
>
> > > + }
>
> > > +
>
> > > + if (tofind == NULL)
>
> > > + {
>
> > > + find_printf(message_parameter_invalid);
>
> > > + return 2;
>
> > > + }
>
> > > +
>
> > > + exitcode = 1;
>
> > > + while (read_line_from_handle(input, &line))
>
> > > + {
>
> > > + if (run_find_for_line(line, tofind))
>
> > > + exitcode = 0;
>
> > > +
>
> > > + heap_free(line);
>
> > > + }
>
> >
>
> > I don't understand why you're reading from stdin uncoditionally. This
>
> > utility takes options, string to find, and a list of file paths. Only
>
> > when paths are not provided it should read from stdin. Will this work if
>
> > you have your string argument starts with '/'?
>
> It's only supposed to be a first implementation, I don't want to add 
> everything in one huge commit. It's already gotten too big for my 
> likes, I don't think we should add even more functionality for the 
> first version.
>

I'm not saying it should support everything from the start, but handling 
of command line arguments is important. It should print some warnings 
and/or abort if file paths are supplied, otherwise it will silently 
return nothing.

> Regards,
>
> Fabian Maurer
>