[clang-tools-extra] r209141 - Improved llvm-namespace-comment check.

[Alexander Kornienko]

On Wed, May 21, 2014 at 7:22 AM, Sean Silva <chisophugis at gmail.com> wrote:

> On Tue, May 20, 2014 at 7:34 PM, Alexander Kornienko <alexfh at google.com>wrote:
>
>> We use lit tests for diagnostic messages. Currently they use FileCheck,
>> but it probably makes sense to implement an analog of Clang's -verify in
>> clang-tidy.
>>
>> As for the tests for fix-its, I consider unit tests to be more readable
>> for this purpose.
>>
>
> I don't understand this. What could be more readable than something like:
>
> namespace {
> } // namespace asdf
> // CHECK:      namespace {
> // CHECK-NEXT: } // namespace
>
> ?? I certainly prefer it hands-down to:
>
> +  EXPECT_EQ("namespace {\n"
> +            "} // namespace",
> +            runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
> +                                                  "} // namespace asdf"));
>
> (actually I think FileCheck's default matching of allowing a match of a
> partial line could be an issue here, but that can be worked around with a
> regex.
>

Yes, it can be worked around. And the resulting test would look similar to
this:

// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
// RUN: clang-tidy %t.cpp -checks='-*,llvm-namespace-comment' -fix --
// RUN: FileCheck -input-file=%t.cpp -strict-whitespace %s
namespace {
} // namespace asdf
// CHECK-NOT: {{.*}}
// CHECK: {{^namespace {$}}
// CHECK-NEXT: {{^} // namespace$}}
// CHECK-NOT: {{.*}}


This can be simplified by extracting all the RUN: commands to a script
(e.g. check_clang_tidy_fix.sh, which is used
in redundant-smartptr-get-fix.cpp), but then the test will require shell,
which iiuc won't allow it to run on Windows, for example.

And if you compare this version with the unit test above, and consider also
factors like portability, performance, convenience of running a single test
and debugging it, etc., what would you prefer? I would definitely prefer
the unit-test version.

 I think that even adding a CHECK-EXACT directive or similar to support
> this in a very readable way might be worth it for this sort of test)
>
>
>> They are also more isolated and closer to the check itself.
>>
>
> I don't understand this.
>

The less code we run, the easier it is to isolate the problem. I don't say
that the integration tests are not needed, but for the fix-it scenario unit
tests are imo better for the reasons I mentioned above.


>  The check is either 1) performing a particular source code transformation
> or 2) providing a diagnostic message about a questionable construct. In
> case 1) you want to verify that the output takes a particular plaintext
> form, i.e. exactly what a FileCheck does. In case 2) you can avoid rolling
> your own -verify and use the simpler but almost surely sufficient facility
> built into FileCheck
> http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-expressions
>

The FileCheck is easy to use, but it is too permissive. It requires
substantial effort to write stricter tests, that don't allow, for example,
extra warnings in the output or warnings being placed on a wrong line.


>
>
>> In a lit test, if we verify the full text of the output, the FileCheck
>> tests become quite bulky.
>>
>
> Could you give an example?
>

See the corrected version of your example above.


>  It is generally possible to create minimal test cases so that the actual
> output (and the input, in fact) are quite compact. yaml2obj is probably a
> worst case in this respect but we've still found it easy to keep the tests
> small and readable. E.g.
>
>
> http://reviews.llvm.org/diffusion/L/browse/llvm/trunk/test/Object/yaml2obj-elf-symbol-LocalGlobalWeak.yaml
>
> http://reviews.llvm.org/diffusion/L/browse/llvm/trunk/test/Object/yaml2obj-elf-bits-endian.test
>
>
> Another reason to put these checks in the unit tests, is that each new lit
>> test file has non-trivial run-time overhead.
>>
>
> Personally I've never found myself waiting on an incremental lit test run
> (i.e. rerunning a particular failing test or relevant test directory for
> the code I'm working on). I *have* found myself waiting significant amounts
> of time for a unittest (gtest) compile and link in that same scenario.
>

This depends on what you change. If you change just the test, then yes, lit
tests don't require rebuilding binaries. But if you change the tested code,
you wait for recompilation anyway.



>
> -- Sean Silva
>
>
>>
>> On Tue, May 20, 2014 at 7:32 PM, Sean Silva <chisophugis at gmail.com>wrote:
>>
>>> Is there a particular reason that this is being tested with unittests
>>> instead of the usual lit tests? This kind of thing seems like it would be a
>>> lot more readable as a lit test.
>>>
>>> -- Sean Silva
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140521/fba8361c/attachment.html>