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

[Manuel Klimek]

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. 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 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
>
>
>> In a lit test, if we verify the full text of the output, the FileCheck
>> tests become quite bulky.
>>
>
> Could you give an example? 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.
>

Interesting. I have pretty much the reverse experience. I think it's mainly
because running a single gtest is very simple (--gtest_filter=*MyTest*)
while running a single lit test is a nightmare. Even worse, if you have a
FileCheck test that tests many different things (take for example the
static analyzer tests, which I happen to have cursed at in the past weeks),
the only way I found to isolate a single cause for debugging is to
copy-and-paste the code. That's really ineffective. On top of that, the
results of FileCheck tests are often really hard to figure out (you have to
basically look at the command, figure out which file is actually checked,
which is often a copy of a copy of a file somewhere, and then use regexps
on that file to see what might have changed so that whatever was not found
didn't appear).

I grant you that FileCheck tests are nicer to read if the tests are at the
integration level anyway, though, which makes the balance often a tough
call (for example, for clang-format we've found that we'd much rather have
FileCheck tests for most tests nowadays, but haven't gotten around to
converting them yet). So I'm actually with you in this specific case. I'd
want to have people most probably maintaining the code to have some headway
here, as insufficient tests will affect them the most.

And finally, you're probably not trying to run the lit stuff on windows,
where starting a process has a significant overhead.

Cheers,
/Manuel


>
> -- 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
>>>
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140521/28332f90/attachment.html>