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