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

[Sean Silva]

On Wed, May 21, 2014 at 6:51 AM, Alexander Kornienko <alexfh at google.com>wrote:

> 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 agree, with the current state of FileCheck, that is not pretty.

However, I think that by using CHECK-LABEL and adding two features to
FileCheck it could be pretty much as simple and clean as I described. The
two features would be to have ignoring CHECK lines themselves be built-in
to FileCheck (IIRC this has been needed in the past for clang-modernize so
this feature would be a nice addition). A CHECK-EXACT would make matching
the exact text a lot cleaner. But yeah, that requires some work on
FileCheck.


>
>  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.
>

I guess the tradeoffs depend here on the nature of the executable. If you
are having to link all of clang, then you're going to have a big link for
your test executable regardless so I guess the unittests aren't too much of
a drag. My experience with yaml2obj, which is a relatively light program,
was that it allowed an extremely fast rebuild and link compared to the
O(all unittests + gtest) link.

-- Sean Silva


>
>
>
>>
>> -- 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/62c82c69/attachment.html>