Kernel prepatch 3.11-rc4 [LWN.net]

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 12:20 UTC (Mon) by error27 (subscriber, #8346)
In reply to: Kernel prepatch 3.11-rc4 by error27
Parent article: Kernel prepatch 3.11-rc4

Also in the kernel checkpatch.pl doesn't let people do:

if ((rc = lbmRead(log, log->page, &bp)))

Banning these is better than writing Yoda code because it makes the code easier to read and less error prone at the same time.

to post comments

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 7:23 UTC (Tue) by dgm (subscriber, #49227) [Link] (7 responses)

How do you catch this, then:

  some_function(x = what_ever, more_args);

specially when "x = ..." comes from macro expansion?

Yoda conditions are VERY useful. You don't even need an static checker. In forcing yourself to slowdown to write the condition reversed, _you_ will catch the mistake before it even reaches the compiler. Worse is better.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 9:13 UTC (Tue) by mpr22 (subscriber, #60784) [Link] (5 responses)

rvalue-first comparison is a clever and useful workaround, and in the guts of a macro, it's clearly the right thing. It's also deeply jarring when I read it, so I'm not sure it's the right thing in the main body of the code.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 9:59 UTC (Tue) by khim (subscriber, #9252) [Link] (4 responses)

It's obviously wrong thing to do because code readability is more important then code writability: it's written once by one guy but then read by thousands and sometimes refactored (to refactor something you need to first read old code and then write new code thus balance is unchanged).

Tradeoffs may be different in code which you are writting for yourself to only run it once... but you would not believe how often such code survives for years, so I'm not sure there are places where I would like to see Yoda conditions in practice.

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:21 UTC (Wed) by dgm (subscriber, #49227) [Link] (3 responses)

What's the value of code that's easy to read, but incorrect?

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:32 UTC (Wed) by mpr22 (subscriber, #60784) [Link] (2 responses)

Low, possibly negative. But once you fix it, it is code that is both correct and easy to read, making it superior to the code that is correct and hard to read.

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 13:22 UTC (Wed) by dgm (subscriber, #49227) [Link] (1 responses)

For this you first have to spot the error. Using one style you get help from the compiler, while with the other you need angry users and some debugger voodoo.

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 15:25 UTC (Wed) by mpr22 (subscriber, #60784) [Link]

The compiler can be (fairly easily) modified to alert the user to the errors that the order of comparison operands people like me find natural makes possible. People-like-me's brains cannot be easily modified to find the order of comparison operands people-like-you find sufficiently cautious natural.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 10:58 UTC (Tue) by error27 (subscriber, #8346) [Link]

We don't often use a compare as a function parameter in the kernel so it's not a problem in practice. We don't like it when people use assignments as a parameter. If I spot an assignment like that in a patch, I will tell the submitter to fix it in a follow on patch. But now I will write a Smatch check to spot it automatically so I can complain every time.

I have reviewed the kernel patches so far this year. Out of 39,745 total patches, there are 21 patches where only a '=' character changed. There is only one '=' vs '==' typo and this ARM bug will be the second bug like that this year. The remaining 20 bugs from the list are almost all off-by-one bugs.

The '=' vs '==' patch was: 9e48854c58 "drm/tilcdc: Fix an incorrect condition". It was found with by checkpatch.pl. Checkpatch is a mandatory step for patches and it reflects a problem in the process that it was skipped for this patch.

It should also have been caught by GCC except the authors of that driver put extra parenthesis around everything as if they are programming in LISP. In the kernel, LISP-isms are considered bad style. We don't have an automated process to complain about this, but I do try to educate submitters to drivers/staging/ about this.

The tilcdc bug would have been caught by Smatch as well but it is an ARM thing. The new CONFIG_COMPILE_TEST might help with this in the future.

Apparently the driver was tested, but I have to think that it wasn't tested very well. The maintainer, Rob Clark, did not have a board to test it on at the time.