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

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 10:32 AM, Jim Grosbach <grosbach at apple.com>
wrote:
>
> 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.
>
How about "Please do not submit/commit test patches that use grep" and
wipe out all other mention of grep (so that it's ungreppable, *sigh*)
from that document?

Eli

On Jan 17, 2013, at 10:38 AM, Eli Bendersky <eliben at google.com> wrote:
> On Thu, Jan 17, 2013 at 10:32 AM, Jim Grosbach <grosbach at
apple.com> wrote:
>> 
>> 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.
>> 
> 
> How about "Please do not submit/commit test patches that use
grep" and
> wipe out all other mention of grep (so that it's ungreppable, *sigh*)
> from that document?
> 
LOL. Sounds great to me. :)

Thanks, Eli.


-Jim