[RFC] cmd: Add a space at the end of the first echo'ed batch line [try5]
Hi everyone, I'm trying to fix the wine cmd behavior but as this 5th try differs greatly with the previous one, I'd like to have some feedback before submitting it. If you have any question, just let me know. Thanks is advance.
On 11/01/2010 07:37 AM, GOUJON Alexandre wrote:
Hi everyone,
I'm trying to fix the wine cmd behavior but as this 5th try differs greatly with the previous one, I'd like to have some feedback before submitting it.
If you have any question, just let me know. Thanks is advance.
+ static const char escaped_space[] = "@space@"; + DWORD len_space = strlen(escaped_space); The better way to calculate size of a static string, which is a compile time calculation. strlen() call is a runtime. You should move declaration from compare_line() and use it instead here.
+static char* replace_escaped_spaces(const char *data, DWORD size, DWORD *new_size) + char *a, *b, *new_data; + a = b = (char*)data; a, b should be "const char*" as well.
+ new_data = (char*)malloc(size*sizeof(char)); Don't use malloc in Wine. Use HeapAlloc & co. This is not c++, don't need to typecast (void*) pointer.
+ new_size += b-a +1; You are modifying the new_size pointer instead of value it points to.
+ if(!run_cmd(actual_cmd_data, actual_cmd_size)) return; You leaking actual_cmd_data here.
+ if(actual_cmd_data) + free(actual_cmd_data); Don't need to check if pointer is NULL before freeing it. free() and HeapFree() do that already.
Vitaliy.
On 11/01/2010 03:12 PM, Vitaliy Margolen wrote:
+ static const char escaped_space[] = "@space@"; + DWORD len_space = strlen(escaped_space); The better way to calculate size of a static string, which is a compile time calculation. strlen() call is a runtime. The better way is ... ? In compare_line(), sizeof(space_cmd) is used but I guess sizeof(space_cmd/space_cmd[0]) is more portable, isn't it ? I thought strlen() was optimized in that case because used with a const string.
+static char* replace_escaped_spaces(const char *data, DWORD size, DWORD *new_size) + char *a, *b, *new_data; + a = b = (char*)data; a, b should be "const char*" as well. Unfortunately, I can't do that : strncpy and HeapFree complains about const strings.
+ new_data = (char*)malloc(size*sizeof(char)); Don't use malloc in Wine. Use HeapAlloc & co. This is not c++, don't need to typecast (void*) pointer. Got it. I'll remember this for next times.
I modified my patch according to your suggestions. I also changed the way I update new_data to simply return it (more understandable, I think). Can you review it again, please ?
Vitaliy. Many thanks, I'm avoiding try[6-9] with your advices !!
GOUJON Alexandre <ale.goujon(a)gmail.com> wrote:
On 11/01/2010 03:12 PM, Vitaliy Margolen wrote:
+ static const char escaped_space[] = "@space@"; + DWORD len_space = strlen(escaped_space); The better way to calculate size of a static string, which is a compile time calculation. strlen() call is a runtime. The better way is ... ? In compare_line(), sizeof(space_cmd) is used but I guess sizeof(space_cmd/space_cmd[0]) is more portable, isn't it ? I thought strlen() was optimized in that case because used with a const string.
Only if you expect that a compiler knows what strlen() is. Better to not rely on internal compiler knowledge.
+static char* replace_escaped_spaces(const char *data, DWORD size, DWORD *new_size) + char *a, *b, *new_data; + a = b = (char*)data; a, b should be "const char*" as well. Unfortunately, I can't do that : strncpy and HeapFree complains about const strings.
In general never cast away const. I don't see where you are using 'a' or 'b' in strncpy() or HeapFree() as a target pointer that prevents making them const, only 'new_data' needs to be non-const.
+ a = b += len_space;
This is absolutely unreadable. Also if you could add some spaces around '+'/'-'/'=' in the cases below it would improve readability as well:
+ *new_size=0;
+ strncpy(new_data+*new_size, a, b-a+1); + *new_size += b-a +1; + new_data[*new_size-1] = ' ';
+ strncpy(new_data+*new_size, a, strlen(a)+1);
etc. -- Dmitry.
Hi Alexandre, On 11/1/10 4:23 PM, GOUJON Alexandre wrote:
+/* Substitute escaped spaces with real ones */ +static char* replace_escaped_spaces(const char *data, DWORD size, DWORD *new_size)
Why do you need this? AFAICS the change to compare_line should be enough. Jacek
On 11/01/2010 10:47 PM, Jacek Caban wrote: > Why do you need this? AFAICS the change to compare_line should be enough. In wine/programs/cmd/tests there are 2 files : + test_builtins.cmd + test_builtins.cmd.exp The first one is a kind of .bat on windows. The wine cmd read line after line and execute it. Then, the output is compared to the content of the second one. I know the title of my patch may be unclear but it's really what windows cmd does : cmd_prompt>echo a(a)space@ a A space is added at the end of the line. That @space@ is added in the test_builtins.cmd.exp However, to extend tests, I added several lines in test_builtins.cmd with terminating spaces. There, AJ warned me that git will ignore them as they are trailing spaces. He tolds me to find a way to add these spaces. I first tried ^z (an ASCII char meaning substitute) but he replied me it is considered as a new line on windows. He advise me then to use, as in test_bultins.cmd.exp "@space@" And that's why I have to "parse" the input cmd_data to find and replace @space@ occurences with "real" spaces. I hope it's now clearer.
participants (4)
-
Dmitry Timoshkov -
GOUJON Alexandre -
Jacek Caban -
Vitaliy Margolen