Diego Biurrun <[email protected]> writes:

> On Mon, Jun 06, 2011 at 05:52:16PM +0100, Mans Rullgard wrote:
>> Signed-off-by: Mans Rullgard <[email protected]>
>
> Hmm, well, you are fond of splitting Makefiles and everything
> else is already split so I don't mind.
>
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -181,13 +181,7 @@ uninstall-data:
>>  
>> -clean:: testclean
>> +clean::
>
> What's the benefit of moving the dependency to the subdirectory Makefile?

Keeping everything test related in one place.

>> @@ -201,119 +195,7 @@ distclean::
>>  
>> -check: test checkheaders
>
> I'm not sure that target should be moved to tests/, checkheaders is not
> defined there.

Good point.

>> --- /dev/null
>> +++ b/tests/Makefile
>> @@ -0,0 +1,119 @@
>> +include $(SRC_PATH_BARE)/tests/fate.mak
>> +include $(SRC_PATH_BARE)/tests/fate2.mak
>> +
>> +include $(SRC_PATH_BARE)/tests/fate/aac.mak
>> +include $(SRC_PATH_BARE)/tests/fate/als.mak
>> +include $(SRC_PATH_BARE)/tests/fate/fft.mak
>> +include $(SRC_PATH_BARE)/tests/fate/h264.mak
>> +include $(SRC_PATH_BARE)/tests/fate/mp3.mak
>> +include $(SRC_PATH_BARE)/tests/fate/vorbis.mak
>> +include $(SRC_PATH_BARE)/tests/fate/vp8.mak
>
> unrelated: 
>
>   include $(SRC_PATH_BARE)/tests/fate/*.mak

Bad idea.

>> +FATE = $(FATE_ACODEC)                                                   \
>> +       $(FATE_VCODEC)                                                   \
>> +       $(FATE_LAVF)                                                     \
>> +       $(FATE_LAVFI)                                                    \
>> +       $(FATE_SEEK)                                                     \
>
> extra karma: Maybe you can cut down on that silly amount of whitespace
> while moving the line anyway.

Does it bother you?

>> +ifdef SAMPLES
>> +FATE += $(FATE_TESTS)
>
> unrelated: This reminds me that I'd prefer for SAMPLES to be renamed to
> FATE_SAMPLES, just as the environment variable.

Why?  Do you enjoy typing?

-- 
Måns Rullgård
[email protected]
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to