Diego Biurrun <[email protected]> writes: > On Mon, May 16, 2011 at 01:11:10AM +0100, Måns Rullgård wrote: >> Diego Biurrun <[email protected]> writes: >> >> > On Sun, May 15, 2011 at 11:34:35PM +0100, Måns Rullgård wrote: >> >> Diego Biurrun <[email protected]> writes: >> >> >> >> > On Sun, May 15, 2011 at 07:21:59PM +0100, Måns Rullgård wrote: >> >> >> Diego Biurrun <[email protected]> writes: >> >> >> > >> >> >> > --- a/libavcodec/fft-internal.h >> >> >> > +++ b/libavcodec/fft-internal.h >> >> >> > @@ -19,6 +19,8 @@ >> >> >> > #ifndef AVCODEC_FFT_INTERNAL_H >> >> >> > #define AVCODEC_FFT_INTERNAL_H >> >> >> > >> >> >> > +#include "fft.h" >> >> >> > + >> >> >> > #if CONFIG_FFT_FLOAT >> >> >> >> >> >> Are you sure this doesn't break something? That header is one of those >> >> >> slightly magical ones. >> >> > >> >> > It does not break FATE. What else do you want me to test for? >> >> >> >> I want you to read and understand what the file does, not just blindly >> >> run some tests and hope they'd catch whatever might have gone wrong. >> > >> > It is safe. First, fft-internal uses struct definitions and similar >> > from fft.h, so it implicitly depends on that header. Second, the only >> > risky part in fft.h is the CONFIG_FFT_FLOAT definition that might conflict >> > with a similar definition elsewhere. >> > >> > However, the definition in fft.h is conditional on CONFIG_FFT_FLOAT >> > being undefined and the only places where fft-internal.h is #included, >> > mdct.c and fft.c, are template files #included from their fixed/float >> > counterparts that set the CONFIG_FFT_FLOAT definition appropriately >> > before anything else. >> > >> > Is that enough to convince you? >> >> No, but had you pointed out that the only places it is included, fft.h >> is included immediately before, it would have been. > > Very well, noted for the future - is that an OK for the patch?
No. >> However, this particular header makes no sense including on its own. >> What is the point in making it appear to work? > > I want to add checkheaders (and a few other targets) to FATE, they get > continuously broken otherwise. But obviously a prerequisite for that > is having the checkheaders target pass in the first place. Blindly adding #include lines isn't necessarily the proper way to do that. >> If you want to juggle #include lines, remove the useless ones at the end >> of avutil.h and fix the fallout instead. That would remove a huge >> number of false dependencies. > > I'll create a branch for that, but I cannot promise an ETA. Getting rid of those #includes is more important than making checkheaders pass since it is effectively a slight API change (some app might be relying on the indirect inclusion), and we should thus hurry up and do it now while things are still considered unstable. Having checkheader is nice, but only that. It doesn't solve any real problems. -- Måns Rullgård [email protected] _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
