Warning on strange indent

Hi,

motivated by the current libsecurity_ssl brouhaha, I experimented with giving clang a -Windent that warns on strange indent. For performance and other reasons, it it only compares the indent of the first statement after a non-composite if/for/while with the indent of the if/for/while itself, and warns if the statement following the if/for/while (not the child of the if/for/while, but the statement after that) has a higher indent than the if/for/while. So it’d warn on this:

if (1)
do_stuff();
do_stuff(); // Has higher indent than the if

This works better than I thought it would – I tried it on chromium, and while it fires quite a bit, when it fires it’s justified most of the time. It’s probably too loud to be enabled by default, but it should do well in nicely-formatted codebases such as llvm, webkit, chromium’s non-third-party code, etc.

I’m away for the next week, but maybe someone wants to pick it up. The patch and many code snippets that are tricky are in http://llvm.org/bugs/show_bug.cgi?id=18938 .

(I also tried a different approach that didn’t work as well, see bug.)

Nico

I can’t help with the work itself, but as a developer I’d love to see something like this in Clang. It would be very very useful.

I can see an argument for this being stylistic, but as an off-by-default option, I see no real downside to including it.

Philip

I can't help with the work itself, but as a developer I'd love to see something like this in Clang. It would be very very useful.

I can see an argument for this being stylistic, but as an off-by-default option, I see no real downside to including it.

Yes, in fact there are already whitespace-sensitive warning diagnostics that are on by default:

$ cat ws.cpp
void f() {
for (;;);
return;
}

$ clang ws.cpp
ws.cpp:2:11: warning: for loop has empty body [-Wempty-body]
for (;;);
^
ws.cpp:2:11: note: put the semicolon on a separate line to silence this warning
1 warning generated.

It'll be a good idea to group these together under a warning category so they can be disabled by tools like ccache that compress or modify the user's whitespace, say -Wno-significant-whitespace.

Alp.

The one that would have been trivially avoided by a coding convention that said that if statements must always have their bodies enclosed in braces? A rule that's been recommended by CERT and others for a very long time? A rule that also avoids the dangling else problem? A rule that is also the opposite of what LLVM recommends? A rule whose lack has already caused subtle bugs in LLVM (I've fixed a couple of them in the past, I don't know how many other people have encountered)?

David
Who thinks coding styles should be designed to discourage bugs, not encourage them.

Yes, and I even agree with you. Having said that, legacy code doesn't always comply and I'm not yet willing to run clang-format at that kind of scale. Or get into the argument about style frankly.

Philip