<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, May 23, 2014 at 5:37 AM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div><div class="h5">On Thu, May 22, 2014 at 2:06 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div>On Thu, May 22, 2014 at 1:09 AM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div><div>On Wed, May 21, 2014 at 6:51 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>On Wed, May 21, 2014 at 7:22 AM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>On Tue, May 20, 2014 at 7:34 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>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.</div>
<div><br></div>As for the tests for fix-its, I consider unit tests to be more readable for this purpose.</div></blockquote><div><br></div></div><div>I don't understand this. What could be more readable than something like:</div>
<div><br></div><div><div>namespace {</div><div>} // namespace asdf</div><div>// CHECK: namespace {</div><div>// CHECK-NEXT: } // namespace</div></div><div><br></div><div>?? I certainly prefer it hands-down to:</div>
<div><div>
<br></div><div><div>+ EXPECT_EQ("namespace {\n"</div><div>+ "} // namespace",</div><div>+ runCheckOnCode<NamespaceCommentCheck>("namespace {\n"</div><div>+ "} // namespace asdf"));</div>
</div><div><br></div></div><div>(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.</div></div></div></div></blockquote>
<div><br></div></div><div>Yes, it can be worked around. And the resulting test would look similar to this:</div><div><br></div></div></div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div class="gmail_extra">
<div class="gmail_quote"><div><div><font face="courier new, monospace">// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp</font></div></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><div><font face="courier new, monospace">// RUN: clang-tidy %t.cpp -checks='-*,llvm-namespace-comment' -fix --</font></div>
</div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><div><font face="courier new, monospace">// RUN: FileCheck -input-file=%t.cpp -strict-whitespace %s</font></div></div></div></div><div class="gmail_extra">
<div class="gmail_quote"><div><font face="courier new, monospace">namespace {</font></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><div><font face="courier new, monospace">} // namespace asdf</font></div>
</div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><div><font face="courier new, monospace">// CHECK-NOT: {{.*}}</font></div></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div>
<div>
<font face="courier new, monospace">// CHECK: {{^namespace {$}}</font></div></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><div><font face="courier new, monospace">// CHECK-NEXT: {{^} // namespace$}}</font></div>
</div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace">// CHECK-NOT: {{.*}}</font></div></div></div></blockquote><div class="gmail_extra"><div class="gmail_quote"><div>
<br></div><div>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.</div>
<div><br></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div></div><div>I agree, with the current state of FileCheck, that is not pretty.</div><div><br></div><div>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.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div> 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)</div>
<div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">They are also more isolated and closer to the check itself.</div>
</blockquote><div><br></div></div><div>I don't understand this.</div></div></div></div></div></blockquote><div><br></div></div><div>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.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div> 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 <a href="http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-expressions" target="_blank">http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-expressions</a></div>
</div></div></div></div></blockquote><div><br></div></div><div>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.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"> In a lit test, if we verify the full text of the output, the FileCheck tests become quite bulky.</div>
</blockquote><div> </div></div></div><div>Could you give an example?</div></div></div></div></blockquote><div><br></div></div><div>See the corrected version of your example above.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> 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.</div>
<div><br></div><div><a href="http://reviews.llvm.org/diffusion/L/browse/llvm/trunk/test/Object/yaml2obj-elf-symbol-LocalGlobalWeak.yaml" target="_blank">http://reviews.llvm.org/diffusion/L/browse/llvm/trunk/test/Object/yaml2obj-elf-symbol-LocalGlobalWeak.yaml</a><br>
</div><div><a href="http://reviews.llvm.org/diffusion/L/browse/llvm/trunk/test/Object/yaml2obj-elf-bits-endian.test" target="_blank">http://reviews.llvm.org/diffusion/L/browse/llvm/trunk/test/Object/yaml2obj-elf-bits-endian.test</a><br>
</div><div>
<div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Another reason to put these checks in the unit tests, is that each new lit test file has non-trivial run-time overhead.</div>
</blockquote><div><br></div></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div></div><div>I usually just build the unit tests I'm about to run? My cycle looks like:</div><div>ninja MyUnitTests && ./path/to/MyUnitTests</div></div></div>
</div></blockquote><div><br></div></div></div><div>I'll need to reanalyze the situation. IIRC somewhere in my past I tracked down a really significant wait on linking gtest itself (independently of any actual tests, which in this case add even more time since all tests are linked into the same executable), and I think that experience has made me a bit gtest averse. Out of curiosity, with a typical clang-tidy or clang-format unittest, how long does "ninja MyUnitTests && ./path/to/MyUnitTests" take on your machine? I think part of my problem may be that my machine here at home might be in need of an upgrade...</div>
</div></div></div></blockquote><div><br></div><div>(changing something in clang-format first of course ;)</div><div>$ time ( ninja FormatTests && tools/clang/unittests/Format/FormatTests )<br></div><div><div>real 0m4.555s</div>
<div>user 0m4.080s</div><div>sys 0m0.390s</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="">
<div><br></div><div>-- Sean Silva</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888">
<div><br></div><div>-- Sean Silva</div></font></span><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888">
<div><br></div><div>-- Sean Silva</div></font></span><div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">
<div><div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, May 20, 2014 at 7:32 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">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.<span><font color="#888888"><div>
<br></div><div>-- Sean Silva</div></font></span></div></blockquote></div>
</div></div></div></div>
</blockquote></div></div></div></div></blockquote></div></div><div dir="ltr"><br>
</div>
</div></div>
</blockquote></div></div><br></div></div>
<br></div><div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></div></blockquote></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div></div>