Diego Biurrun <[email protected]> writes:
> On Tue, Aug 28, 2012 at 01:54:57PM +0100, Måns Rullgård wrote:
>> Diego Biurrun <[email protected]> writes:
>> >
>> > --- a/configure
>> > +++ b/configure
>> > @@ -1207,7 +1221,7 @@ HAVE_LIST="
>> > yasm
>> > "
>> >
>> > -# options emitted with CONFIG_ prefix but not available on command line
>> > +# options emitted with CONFIG_/HAVE_ prefix but not available on command
>> > line
>> > CONFIG_EXTRA="
>>
>> That comment change makes no sense.
>
> Umm, I add HAVE_EXTRA directly below CONFIG_EXTRA ...
Then the comment should be there too. But the whole thing is
unnecessary anyway per my comments that you snipped.
>> > @@ -1240,6 +1254,14 @@ CONFIG_EXTRA="
>> >
>> > +ARCH_EXT_LIST_X86_INLINE=$(add_prefix inline_ $ARCH_EXT_LIST_X86)
>> > +ARCH_EXT_LIST_X86_YASM=$(add_prefix yasm_ $ARCH_EXT_LIST_X86)
>>
>> What, no vertical alignment?
>
> You know very well that it's overrated ;)
>
> It's kinda hard in shell as the syntax disallows spaces before = in
> assignments.
Yes, but it does allow extra spaces inside $().
>> > @@ -1863,7 +1917,10 @@ for n in $COMPONENT_LIST; do
>> > eval ${n}_if_any="\$$v"
>> > done
>> >
>> > -enable $ARCH_EXT_LIST $ALL_TESTS
>> > +enable $ARCH_EXT_LIST \
>> > + $ARCH_EXT_LIST_X86_INLINE \
>> > + $ARCH_EXT_LIST_X86_YASM \
>> > + $ALL_TESTS \
>>
>> This will get out of hand quickly if we want to do something similar for
>> other architectures. Could the same thing be achieved by in the loop I
>> propose above adding this line:
>>
>> eval ${ext}_suggest='"inline_$ext yasm_$ext"'
>>
>> For future expansion, we might also want to use something more generic
>> than yasm in these variable names.
>
> We have already talked about naming surrounding assemblers/assembly
> without tangible results. The best I can come up with are
> "inline_asm" and "standalone_asm", do you have a better suggestion?
external?
Details aside, it just occurred to me that naming these variables
HAVE_${EXT}_INLINE etc (instead of HAVE_INLINE_${EXT}) makes it easier
to grep for HAVE_${EXT} and get sane results. It also feels a bit more
logical that way.
>> > --- a/libavfilter/x86/gradfun.c
>> > +++ b/libavfilter/x86/gradfun.c
>> > @@ -24,12 +24,10 @@
>> > #include "libavutil/x86/asm.h"
>> > #include "libavfilter/gradfun.h"
>> >
>> > -#if HAVE_INLINE_ASM
>>
>> Why do you remove that line?
>
> I'm replacing HAVE_FOO below with HAVE_INLINE_FOO, which already
> ensures that HAVE_INLINE_ASM is set.
But now you're defining those constants even if it's not enabled.
>> > --- a/libavutil/x86/float_dsp_init.c
>> > +++ b/libavutil/x86/float_dsp_init.c
>> > @@ -33,16 +33,14 @@ extern void ff_vector_fmac_scalar_avx(float *dst,
>> > const float *src, float mul,
>> >
>> > void ff_float_dsp_init_x86(AVFloatDSPContext *fdsp)
>> > {
>> > -#if HAVE_YASM
>> > int mm_flags = av_get_cpu_flags();
>> >
>> > - if (mm_flags & AV_CPU_FLAG_SSE && HAVE_SSE) {
>> > + if (mm_flags & AV_CPU_FLAG_SSE && HAVE_YASM_SSE) {
>> > fdsp->vector_fmul = ff_vector_fmul_sse;
>> > fdsp->vector_fmac_scalar = ff_vector_fmac_scalar_sse;
>> > }
>> > - if (mm_flags & AV_CPU_FLAG_AVX && HAVE_AVX) {
>> > + if (mm_flags & AV_CPU_FLAG_AVX && HAVE_YASM_AVX) {
>> > fdsp->vector_fmul = ff_vector_fmul_avx;
>> > fdsp->vector_fmac_scalar = ff_vector_fmac_scalar_avx;
>> > }
>> > -#endif
>> > }
>>
>> Why do you remove the #if HAVE_YASM? I imagine it's there for a reason.
>
> Same reasoning here, I'm replacing it with something more specific.
> Note that the code still compiles with --disable-yasm.
And no warnings either?
--
Måns Rullgård
[email protected]
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
