I agree @todo_space@ should go; it's never used, so it's dead code now. Tempting to say that removing it should be a separate patch, rather than combining that with adding a new feature. Since all the @keywords@ are poorly documented, I'm glad to see an unused one go.
Your generic @todo_wine@ keyword seems useful.
The current code is a bit ugly. If you're going to add a nice is_todo_wine_line() helper, it might make sense to do a cleanup patch that cleans up the @keyword@ recognition in general first. (Why the verbose array declaration instead of a "constant"? etc.)
I see you're making every line of the .exp file count as a test. That seems ok.
There's a fair bit of code duplication; I wonder if you could avoid that by refactoring things a bit.
To avoid adding dead code, it might be nice to see a test that actually uses your new keyword. - Dan
2011/6/13 Dan Kegel dank@kegel.com:
I agree @todo_space@ should go; it's never used, so it's dead code now. Tempting to say that removing it should be a separate patch, rather than combining that with adding a new feature.
Well, maybe but I didn't see its usefulness when @todo_wine@ is introduced. It could help for regression testing, admittedly
Since all the @keywords@ are poorly documented, I'm glad to see an unused one go.
Your generic @todo_wine@ keyword seems useful.
The current code is a bit ugly. If you're going to add a nice is_todo_wine_line() helper, it might make sense to do a cleanup patch that cleans up the @keyword@ recognition in general first. (Why the verbose array declaration instead of a "constant"? etc.)
Those keywords can appear anywhere on the line and act basically as a search&replace (except @or_broken@) while the @todo_wine@ must be used at the start of a line to be recognized, and doesn't generate any text. But OK an array seemed odd to me as well... it was probably done so that sizeof(foo_cmd) could be used in memcp(expected_ptr, foo_cmd, sizeof(foo_cmd)).
Maybe it was done to optimise stuff since sizeof(foo_cmd) would be computed at compile time, but I don't see why strlen() would not be for static string constants.
As for the helper, I originally thought about adding a param to compare_line so that
static const char *compare_line(const char *out_line, const char *out_end, const char *exp_line, const char *exp_end)
becomes
static const char *compare_line(const char *out_line, const char *out_end, const char *exp_line, const char *exp_end, BOOL* is_todo_line)
There would then be no need of a is_todo_wine_line() helper. Would that be better?
I see you're making every line of the .exp file count as a test. That seems ok.
That's actually how it currently works: I didn't change that behaviour, but added a check fo
There's a fair bit of code duplication; I wonder if you could avoid that by refactoring things a bit.
Maybe but I wanted to keep this patch as
To avoid adding dead code, it might be nice to see a test that actually uses your new keyword.
I've made several mkdir tests with it, and it seems to work OK. I was waiting to see if this patch is commited before I submit. You can see it here: http://pastebin.com/tnKY4Q7S
Dan
Frédéric
2011/6/13 Frédéric Delanoy frederic.delanoy@gmail.com:
But OK an array seemed odd to me as well... it was probably done so that sizeof(foo_cmd) could be used in memcp(expected_ptr, foo_cmd, sizeof(foo_cmd)).
Nah, looking at the code, it was so that sizeof() would not include the trailing NUL.
But compare_string is just ugly. Looking at it a bunch, I suspect we should factor out @keyword@ expansion into a separate function; then we could have better error messages that gave the whole expected line. I might send a patch in like that soon.
In the meantime, please either send a separate patch that removes todo_space, or just leave todo_space alone for now.
To avoid adding dead code, it might be nice to see a test that actually uses your new keyword.
I've made several mkdir tests with it, and it seems to work OK. I was waiting to see if this patch is commited before I submit. You can see it here: http://pastebin.com/tnKY4Q7S
I'd like to see the code that implements @todo_wine@ submitted in the same patch as the first test that uses it. - Dan
2011/6/13 Dan Kegel dank@kegel.com:
In the meantime, please either send a separate patch that removes todo_space, or just leave todo_space alone for now.
To avoid adding dead code, it might be nice to see a test that actually uses your new keyword.
I've made several mkdir tests with it, and it seems to work OK. I was waiting to see if this patch is commited before I submit. You can see it here: http://pastebin.com/tnKY4Q7S
I'd like to see the code that implements @todo_wine@ submitted in the same patch as the first test that uses it.
- Dan
I'll probably send them in a patch series; should be good enough, no?
2011/6/13 Frédéric Delanoy frederic.delanoy@gmail.com:
I'd like to see the code that implements @todo_wine@ submitted in the same patch as the first test that uses it.
I'll probably send them in a patch series; should be good enough, no?
Yes, that's fine.
-
Dan Kegel -
Frédéric Delanoy