https://fedorahosted.org/sssd/ticket/871
The second patch adds a couple of unit tests to exercise both the old behaviour and the multiple spaces.
On Thu, 2011-05-26 at 18:15 +0200, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/871
The second patch adds a couple of unit tests to exercise both the old behaviour and the multiple spaces.
Patch 0001: Nack.
"if (w) w = 0;" is redundant. "w = 0;" is sufficient here.
Also, while it's unlikely to matter, this code only handles the single whitespace code. It would be wiser if we used isspace() instead.
For readability purposes, please convert "i", "e" and "w" to the "bool" type and use true/false.
Finally, please comment this code. It's really difficult to read.
Patch 0002: Please add test cases for tabs.
On 05/26/2011 06:56 PM, Stephen Gallagher wrote:
On Thu, 2011-05-26 at 18:15 +0200, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/871
The second patch adds a couple of unit tests to exercise both the old behaviour and the multiple spaces.
Patch 0001: Nack.
"if (w) w = 0;" is redundant. "w = 0;" is sufficient here.
Also, while it's unlikely to matter, this code only handles the single whitespace code. It would be wiser if we used isspace() instead.
For readability purposes, please convert "i", "e" and "w" to the "bool" type and use true/false.
Finally, please comment this code. It's really difficult to read.
Patch 0002: Please add test cases for tabs.
Done except converting "i" to "bool" - that can't be done, "i" is an index, not a flag.
On Fri, 2011-05-27 at 14:28 +0200, Jakub Hrozek wrote:
On 05/26/2011 06:56 PM, Stephen Gallagher wrote:
On Thu, 2011-05-26 at 18:15 +0200, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/871
The second patch adds a couple of unit tests to exercise both the old behaviour and the multiple spaces.
Patch 0001: Nack.
"if (w) w = 0;" is redundant. "w = 0;" is sufficient here.
Also, while it's unlikely to matter, this code only handles the single whitespace code. It would be wiser if we used isspace() instead.
For readability purposes, please convert "i", "e" and "w" to the "bool" type and use true/false.
Finally, please comment this code. It's really difficult to read.
Patch 0002: Please add test cases for tabs.
Done except converting "i" to "bool" - that can't be done, "i" is an index, not a flag.
Right, I was overzealous on "i".
Nack. You added a comment /* Add escaped whitespace literally */ and then immediately ignore what it's saying :) You assign tmp[i] to be a single-space character instead of the literal whitespace character received.
On 05/27/2011 02:35 PM, Stephen Gallagher wrote:
On Fri, 2011-05-27 at 14:28 +0200, Jakub Hrozek wrote:
On 05/26/2011 06:56 PM, Stephen Gallagher wrote:
On Thu, 2011-05-26 at 18:15 +0200, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/871
The second patch adds a couple of unit tests to exercise both the old behaviour and the multiple spaces.
Patch 0001: Nack.
"if (w) w = 0;" is redundant. "w = 0;" is sufficient here.
Also, while it's unlikely to matter, this code only handles the single whitespace code. It would be wiser if we used isspace() instead.
For readability purposes, please convert "i", "e" and "w" to the "bool" type and use true/false.
Finally, please comment this code. It's really difficult to read.
Patch 0002: Please add test cases for tabs.
Done except converting "i" to "bool" - that can't be done, "i" is an index, not a flag.
Right, I was overzealous on "i".
Nack. You added a comment /* Add escaped whitespace literally */ and then immediately ignore what it's saying :) You assign tmp[i] to be a single-space character instead of the literal whitespace character received.
New patches attached with a unit test that exercises the above issue.
On 05/27/2011 02:35 PM, Stephen Gallagher wrote:
On Fri, 2011-05-27 at 14:28 +0200, Jakub Hrozek wrote:
On 05/26/2011 06:56 PM, Stephen Gallagher wrote:
On Thu, 2011-05-26 at 18:15 +0200, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/871
The second patch adds a couple of unit tests to exercise both the old behaviour and the multiple spaces.
Patch 0001: Nack.
"if (w) w = 0;" is redundant. "w = 0;" is sufficient here.
Also, while it's unlikely to matter, this code only handles the single whitespace code. It would be wiser if we used isspace() instead.
For readability purposes, please convert "i", "e" and "w" to the "bool" type and use true/false.
Finally, please comment this code. It's really difficult to read.
Patch 0002: Please add test cases for tabs.
Done except converting "i" to "bool" - that can't be done, "i" is an index, not a flag.
Right, I was overzealous on "i".
Nack. You added a comment /* Add escaped whitespace literally */ and then immediately ignore what it's saying :) You assign tmp[i] to be a single-space character instead of the literal whitespace character received.
New patches attached with a unit test that exercises the above issue.
All commented changes have been done, unit tests work fine.
Ack
Jan
On Fri, 2011-06-10 at 10:43 +0200, Jan Zelený wrote:
On 05/27/2011 02:35 PM, Stephen Gallagher wrote:
On Fri, 2011-05-27 at 14:28 +0200, Jakub Hrozek wrote:
On 05/26/2011 06:56 PM, Stephen Gallagher wrote:
On Thu, 2011-05-26 at 18:15 +0200, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/871
The second patch adds a couple of unit tests to exercise both the old behaviour and the multiple spaces.
Patch 0001: Nack.
"if (w) w = 0;" is redundant. "w = 0;" is sufficient here.
Also, while it's unlikely to matter, this code only handles the single whitespace code. It would be wiser if we used isspace() instead.
For readability purposes, please convert "i", "e" and "w" to the "bool" type and use true/false.
Finally, please comment this code. It's really difficult to read.
Patch 0002: Please add test cases for tabs.
Done except converting "i" to "bool" - that can't be done, "i" is an index, not a flag.
Right, I was overzealous on "i".
Nack. You added a comment /* Add escaped whitespace literally */ and then immediately ignore what it's saying :) You assign tmp[i] to be a single-space character instead of the literal whitespace character received.
New patches attached with a unit test that exercises the above issue.
All commented changes have been done, unit tests work fine.
Ack
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Stephen Gallagher