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

[Sean Silva]

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.

-- 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/20140520/e48b0f23/attachment.html>