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

[Chandler Carruth]

On Wed, May 21, 2014 at 5:09 PM, Sean Silva <chisophugis at gmail.com> wrote:

> 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.
>>
>
Instead you could just teach clang-tidy to output on stdout, and teach
FileCheck to strip the comments it matches from the input (perhaps with a
flag). Then the RUN: lines should boil down to the standard line with a
tool piped to FileCheck...


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

FWIW, I think that doing the work to make tools like this easily testable
with FileCheck would be worthwhile. I like unittests too, and like the fact
that we exercise libraries directly, but I think for a tool which is
fundamentally doing text -> text transformation, it probably makes sense to
use FileCheck.

If anything, I think clang-format is going to be the exception to this rule
because it is *only* changing the whitespace, where as I suspect for
everything other than clang-format we want to ignore whitespace entirely
(because it is fixed and the fixes tested in clang-format) and use
something like the CHECK-EXACT than Sean hypothesizes.

-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140523/46d6717f/attachment.html>