llvm dev - Jan 2013 - [LLVMdev] [llvm-commits] [PATCH] A "very verbose" mode for FileCheck

On Jan 17, 2013, at 9:57 AM, Dmitri Gribenko <gribozavr at gmail.com>
wrote:
> On Thu, Jan 17, 2013 at 7:51 PM, Sean Silva <silvas at purdue.edu>
wrote:
>> On Thu, Jan 17, 2013 at 8:36 AM, Dmitri Gribenko <gribozavr at
gmail.com> wrote:
>>> We have to options:
>>> (a) replace 'FileCheck' with '%FileCheck' in all
tests, and teach
>>> 'lit' to replace '%FileCheck' with 'FileCheck
--dump-input-on-error';
>>> 
>>> (b) teach 'lit' to replace a plain 'FileCheck'.
>>> 
>>> The first approach seems cleaner to developers who read and write
>>> tests (it suggests that they are invoking some "macro" --
but does
>>> that matter?)  The second approach is much easier to implement
since
>>> tests will be unchanged.
>> 
>> IMO the biggest issue with (a) is that developers will continue to use
>> `FileCheck` instead of `%FileCheck`. So IMO (a) should only be
>> implemented if simultaneously there is a change that makes just plain
>> `FileCheck` an error.
> 
> I think that within a month this knowledge will be propagated to all
developers.
I'd like to think so, too, but we still get patches that write tests using
'grep' instead of FileCheck.

-Jim
> 
> Dmitri
> 
> -- 
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at
gmail.com>*/
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On Thu, Jan 17, 2013 at 10:20 AM, Jim Grosbach <grosbach at apple.com>
wrote:
>
> On Jan 17, 2013, at 9:57 AM, Dmitri Gribenko <gribozavr at gmail.com>
wrote:
>
>> On Thu, Jan 17, 2013 at 7:51 PM, Sean Silva <silvas at
purdue.edu> wrote:
>>> On Thu, Jan 17, 2013 at 8:36 AM, Dmitri Gribenko <gribozavr at
gmail.com> wrote:
>>>> We have to options:
>>>> (a) replace 'FileCheck' with '%FileCheck' in
all tests, and teach
>>>> 'lit' to replace '%FileCheck' with
'FileCheck --dump-input-on-error';
>>>>
>>>> (b) teach 'lit' to replace a plain 'FileCheck'.
>>>>
>>>> The first approach seems cleaner to developers who read and
write
>>>> tests (it suggests that they are invoking some
"macro" -- but does
>>>> that matter?)  The second approach is much easier to implement
since
>>>> tests will be unchanged.
>>>
>>> IMO the biggest issue with (a) is that developers will continue to
use
>>> `FileCheck` instead of `%FileCheck`. So IMO (a) should only be
>>> implemented if simultaneously there is a change that makes just
plain
>>> `FileCheck` an error.
>>
>> I think that within a month this knowledge will be propagated to all
developers.
>
> I'd like to think so, too, but we still get patches that write tests
using 'grep' instead of FileCheck.
>
This is unfortunate. Last month I tweaked TestingGuide.rst to
discourage grep in favor of FileCheck. It now says:

"The recommended way to examine output to figure out if the test
passes it using the FileCheck tool. The usage of grep in RUN lines is
discouraged."

However, perhaps it's time to remove any mention of grep from that document?

Eli

On Jan 17, 2013, at 10:30 AM, Eli Bendersky <eliben at google.com> wrote:
> On Thu, Jan 17, 2013 at 10:20 AM, Jim Grosbach <grosbach at
apple.com> wrote:
>> 
>> On Jan 17, 2013, at 9:57 AM, Dmitri Gribenko <gribozavr at
gmail.com> wrote:
>> 
>>> On Thu, Jan 17, 2013 at 7:51 PM, Sean Silva <silvas at
purdue.edu> wrote:
>>>> On Thu, Jan 17, 2013 at 8:36 AM, Dmitri Gribenko <gribozavr
at gmail.com> wrote:
>>>>> We have to options:
>>>>> (a) replace 'FileCheck' with '%FileCheck'
in all tests, and teach
>>>>> 'lit' to replace '%FileCheck' with
'FileCheck --dump-input-on-error';
>>>>> 
>>>>> (b) teach 'lit' to replace a plain
'FileCheck'.
>>>>> 
>>>>> The first approach seems cleaner to developers who read and
write
>>>>> tests (it suggests that they are invoking some
"macro" -- but does
>>>>> that matter?)  The second approach is much easier to
implement since
>>>>> tests will be unchanged.
>>>> 
>>>> IMO the biggest issue with (a) is that developers will continue
to use
>>>> `FileCheck` instead of `%FileCheck`. So IMO (a) should only be
>>>> implemented if simultaneously there is a change that makes just
plain
>>>> `FileCheck` an error.
>>> 
>>> I think that within a month this knowledge will be propagated to
all developers.
>> 
>> I'd like to think so, too, but we still get patches that write
tests using 'grep' instead of FileCheck.
>> 
> 
> This is unfortunate. Last month I tweaked TestingGuide.rst to
> discourage grep in favor of FileCheck. It now says:
> 
> "The recommended way to examine output to figure out if the test
> passes it using the FileCheck tool. The usage of grep in RUN lines is
> discouraged."
> 
> However, perhaps it's time to remove any mention of grep from that
document?
> 
"Usage of grep in RUN lines will result in your patch being reverted."
;)

If you can think of a less snarky way of saying something along those lines,
it'd be great. I'm more than a bit surprised the docs still talk about
it at all, honestly. FileCheck has been the One True Way(™) for quite a while
now.

-Jim

On Thu, Jan 17, 2013 at 1:30 PM, Eli Bendersky <eliben at google.com>
wrote:
> This is unfortunate. Last month I tweaked TestingGuide.rst to
> discourage grep in favor of FileCheck. It now says:
>
> "The recommended way to examine output to figure out if the test
> passes it using the FileCheck tool. The usage of grep in RUN lines is
> discouraged."
>
> However, perhaps it's time to remove any mention of grep from that
document?
One contributing factor to the continued use of grep may be that the
section "Writing New Regression Tests" [1] mentions grep multiple
times, and FileCheck 0 times.

Besides that, I suspect that most new uses of grep come from cargo
culting, so just giving them nothing to cargo cult is an easy and
mechanical way to avoid that part of the issue altogether (i.e. just
removing all uses of grep in the current tests). Also, removing `not`
and `count` from utils/ would incapacitate grep significantly w.r.t.
writing tests with it, discouraging its use and making people look for
a different way.

The cargo culting also points to a lack of adequate documentation
about how to add a new test (`HowToAddATest.rst`? You might be a good
person to write that ;)
<http://www.llvm.org/docs/SphinxQuickstartTemplate.html>). HowTo
documents are a great way to disseminate best practices.

[1] <http://llvm.org/docs/TestingGuide.html#writing-new-regression-tests>

-- Sean Silva