clang-format turning tests into no-ops

Hi Daniel,

Here’s a good one for you…

I’ve recently been fixing a whole lot of lit tests in the LLVM / clang suites that don’t check anything.

It was a mystery why people keep checking in tests like this:

// RUN: clang-check "%s" -- -target x86_64-apple-darwin10 -fasm-blocks 2>&1 |
// ``FileCheck %s

The first line will run successfully, and the second doesn’t begin with RUN so the test passes silently. Not good.

Then it dawned on me – contributors must be running clang-format before committing, causing their RUN lines to get split up at the line boundary.

I know this isn’t strictly a clang-format bug, but it’s time consuming to manually audit and find no-op tests like this after the fact and the failure mode is insidious because these tests will always silently pass.

I was wondering if you can think of a way to exclude the test suites from clang-format and clang-format-diff runs. Guessing this could be tricky given that there are several different ways to run clang-format but something needs to be done…

Alp.

Hi Daniel,

Here's a good one for you..

I've recently been fixing a whole lot of lit tests in the LLVM / clang
suites that don't check anything.

It was a mystery why people keep checking in tests like this:

// RUN: clang-check "%s" -- -target x86_64-apple-darwin10 -fasm-blocks
2>&1 |
// FileCheck %s

The first line will run successfully, and the second doesn't begin with
RUN so the test passes silently. Not good.

Then it dawned on me -- contributors must be running clang-format before
committing, causing their RUN lines to get split up at the line boundary.

I know this isn't strictly a clang-format bug, but it's time consuming to
manually audit and find no-op tests like this after the fact and the
failure mode is insidious because these tests will always silently pass.

Although it doesn't eliminate the hassle of having to manually fix this, it
seems like at least one issue worth fixing in its own right is the fact
that RUN lines ending with a | are silently accepted by our test
infrastructure.

Also, FWIW, it seems like clang-format's comment reflowing turns out to do
the wrong thing in a ton of (most?) scenarios (for example, it will wrap
the last word onto the next line, leaving a bunch of spurious "one word"
lines), to the point that I wish there were an option to tell clang-format
to not mess with comments at all.

-- Sean Silva

That should be easy enough. We simply need a .clang-format file in the test suites removing the column limit.

Done in r194248 and r194221, respectively.

Hi Daniel,

Here's a good one for you..

I've recently been fixing a whole lot of lit tests in the LLVM / clang
suites that don't check anything.

It was a mystery why people keep checking in tests like this:

// RUN: clang-check "%s" -- -target x86_64-apple-darwin10 -fasm-blocks
2>&1 |
// FileCheck %s

The first line will run successfully, and the second doesn't begin with
RUN so the test passes silently. Not good.

Then it dawned on me -- contributors must be running clang-format before
committing, causing their RUN lines to get split up at the line boundary.

I know this isn't strictly a clang-format bug, but it's time consuming to
manually audit and find no-op tests like this after the fact and the
failure mode is insidious because these tests will always silently pass.

Although it doesn't eliminate the hassle of having to manually fix this,
it seems like at least one issue worth fixing in its own right is the fact
that RUN lines ending with a | are silently accepted by our test
infrastructure.

Also, FWIW, it seems like clang-format's comment reflowing turns out to do
the wrong thing in a ton of (most?) scenarios (for example, it will wrap
the last word onto the next line, leaving a bunch of spurious "one word"
lines), to the point that I wish there were an option to tell clang-format
to not mess with comments at all.

clang-format does not "reflow" yet. It just breaks a comment if it is too
long. We'll likely want a "reflow" option, but probably bound to a specific
"editor mode", so that clang-format does not mess up some person's
ASCII-art comment unless it is done in an editor and the result can easily
be undone.

FWIW, I consider a "spurious on word" comment better than a comment
violating the column limit, so I am quite strongly against just turning
comment breaking off.

-- Sean Silva

Hi Daniel,

Here's a good one for you..

I've recently been fixing a whole lot of lit tests in the LLVM / clang
suites that don't check anything.

It was a mystery why people keep checking in tests like this:

// RUN: clang-check "%s" -- -target x86_64-apple-darwin10 -fasm-blocks
2>&1 |
// FileCheck %s

The first line will run successfully, and the second doesn't begin with
RUN so the test passes silently. Not good.

Then it dawned on me -- contributors must be running clang-format before
committing, causing their RUN lines to get split up at the line boundary.

I know this isn't strictly a clang-format bug, but it's time consuming
to manually audit and find no-op tests like this after the fact and the
failure mode is insidious because these tests will always silently pass.

Although it doesn't eliminate the hassle of having to manually fix this,
it seems like at least one issue worth fixing in its own right is the fact
that RUN lines ending with a | are silently accepted by our test
infrastructure.

Also, FWIW, it seems like clang-format's comment reflowing turns out to
do the wrong thing in a ton of (most?) scenarios (for example, it will wrap
the last word onto the next line, leaving a bunch of spurious "one word"
lines), to the point that I wish there were an option to tell clang-format
to not mess with comments at all.

clang-format does not "reflow" yet. It just breaks a comment if it is too
long. We'll likely want a "reflow" option, but probably bound to a specific
"editor mode", so that clang-format does not mess up some person's
ASCII-art comment unless it is done in an editor and the result can easily
be undone.

Ok, that explains it. I guess I was misinterpreting "reflow not
implemented" as "broken".

FWIW, I consider a "spurious on word" comment better than a comment
violating the column limit, so I am quite strongly against just turning
comment breaking off.

Maybe better, but still unacceptable (it's a tough pickle to get out of).
This is a not-that-extreme example of what I have seen (on some random
lorem ipsum text):

// Lorem ipsum dolor sit amet, consectetur adipiscing elit. Maecenas mattis
quam
// non nibh
// pretium ultrices. Etiam iaculis facilisis tristique. Suspendisse at
mattis
// magna.
// Fusce facilisis purus dui, eget porttitor velit porta at. Nulla commodo
// mauris
// porttitor mauris malesuada, quis facilisis justo egestas. Cras laoreet
neque
// est, vel
// congue nisi dapibus mollis. Morbi vehicula auctor libero, sit amet
// pellentesque justo.
// Mauris nisi nisi, sodales id ligula in, mattis sollicitudin lorem. Sed
// eleifend dui
// sit amet arcu vehicula dignissim. Suspendisse at porta felis, ut interdum
// nisl. Donec
// pellentesque justo ac dui sagittis, et facilisis augue fermentum. Fusce
// dapibus
// pulvinar erat et blandit. Praesent in quam non nunc consequat venenatis
ac
// sed ipsum.
// Morbi rutrum leo quis elit iaculis tristique. Curabitur et metus justo.

This kind of comment shape would not be acceptable in any codebase I know
of. I think it could be argued that simply not touching comments is the
better choice, but since it seems like proper reflowing is on the agenda,
it's probably not worth discussing (especially since it's so easy to fix
with `gq` in vim).

-- Sean Silva

In article <527C7B81.3030100@nuanti.com>,
    Alp Toker <alp@nuanti.com> writes:

It was a mystery why people keep checking in tests like this:

>// RUN: clang-check "%s" -- -target x86_64-apple-darwin10 -fasm-blocks
2>&1 |||
>>// ||FileCheck %s|

The first line will run successfully, and the second doesn't begin with
RUN so the test passes silently. Not good.

Then it dawned on me -- contributors must be running clang-format before
committing, causing their RUN lines to get split up at the line boundary.

The more general problem seems to be that not all comments are created
equal.

There should be a way to tell clang-format that comments matching a
certain pattern should be left alone. I can think of any number of
tools that use metadata embedded in comments that should never be
reformatted. These lit test metadata lines are one example and other
examples are:

* comments suppressing cppcheck diagnostics
* comments suppressing lint diagnostics
* comments suppressing coverity diagnostics
* comments indicating editor options (emacs mode?)
* comments containing version control metadata $Id$, etc.

I'm sure we can think of some more examples.

It seems there should be some sort of option providing clang-format
with a pattern that can be matched against a comment line/block that
will tell clang-format to leave the line/block alone. This would also
be useful for marking ASCII art as unmolestable.