vfwcap: Unbreak building after c201069fa

Message ID 1428658006-33955-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö April 10, 2015, 9:26 a.m.
These headers can't be included in any arbitrary (alphabetical) order.
---
 libavdevice/vfwcap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Vittorio Giovara April 10, 2015, 9:38 a.m. | #1
On Fri, Apr 10, 2015 at 11:26 AM, Martin Storsjö <martin@martin.st> wrote:
> These headers can't be included in any arbitrary (alphabetical) order.
> ---
>  libavdevice/vfwcap.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Ok. I added that snippet to https://wiki.libav.org/CodingStyle/HeaderOrdering
Diego Biurrun April 10, 2015, 10:12 a.m. | #2
On Fri, Apr 10, 2015 at 12:26:46PM +0300, Martin Storsjö wrote:
> These headers can't be included in any arbitrary (alphabetical) order.

They're not in arbitrary order, they are in system headers before local
headers order.  If that breaks I think we have some deeper problem in
our libavformat headers.

> --- a/libavdevice/vfwcap.c
> +++ b/libavdevice/vfwcap.c
> @@ -19,9 +19,6 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
> -#include <vfw.h>
> -#include <windows.h>
> -
>  #include "libavutil/internal.h"
>  #include "libavutil/log.h"
>  #include "libavutil/opt.h"
> @@ -30,6 +27,12 @@
>  #include "libavformat/avformat.h"
>  #include "libavformat/internal.h"
>  
> +// windows.h should be included after winsock2.h (if not, it may trigger
> +// warnings but not errors), and libavformat internals may include winsock2.h
> +#include <windows.h>
> +// windows.h needs to be inclued before vfw.h

inclu_d_ed

Diego
Martin Storsjö April 10, 2015, 10:25 a.m. | #3
On Fri, 10 Apr 2015, Diego Biurrun wrote:

> On Fri, Apr 10, 2015 at 12:26:46PM +0300, Martin Storsjö wrote:
>> These headers can't be included in any arbitrary (alphabetical) order.
>
> They're not in arbitrary order, they are in system headers before local
> headers order.  If that breaks I think we have some deeper problem in
> our libavformat headers.

It's two separate issues:

1) You can't include vfw.h before windows.h, that breaks builds. Did you 
look at fate lately? It's kinda red-ish. Including these two in 
alphabetical order with respect to each other breaks things.

2) Including windows.h before winsock2.h will give compiler warnings with 
mingw, and breaks the build with MSVC. libavformat/internal.h will include 
winsock2.h

Therefore you can't enforce the rule with system headers before local 
headers as long as windows.h is involved. Yes, it's of course ugly that 
their system headers aren't self-sufficient, but that's the way it is.

>
>> --- a/libavdevice/vfwcap.c
>> +++ b/libavdevice/vfwcap.c
>> @@ -19,9 +19,6 @@
>>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>   */
>> 
>> -#include <vfw.h>
>> -#include <windows.h>
>> -
>>  #include "libavutil/internal.h"
>>  #include "libavutil/log.h"
>>  #include "libavutil/opt.h"
>> @@ -30,6 +27,12 @@
>>  #include "libavformat/avformat.h"
>>  #include "libavformat/internal.h"
>> 
>> +// windows.h should be included after winsock2.h (if not, it may trigger
>> +// warnings but not errors), and libavformat internals may include winsock2.h
>> +#include <windows.h>
>> +// windows.h needs to be inclued before vfw.h
>
> inclu_d_ed

Fixed locally.

// Martin
Diego Biurrun April 10, 2015, 10:32 a.m. | #4
On Fri, Apr 10, 2015 at 01:25:43PM +0300, Martin Storsjö wrote:
> On Fri, 10 Apr 2015, Diego Biurrun wrote:
> 
> >On Fri, Apr 10, 2015 at 12:26:46PM +0300, Martin Storsjö wrote:
> >>These headers can't be included in any arbitrary (alphabetical) order.
> >
> >They're not in arbitrary order, they are in system headers before local
> >headers order.  If that breaks I think we have some deeper problem in
> >our libavformat headers.
> 
> It's two separate issues:
> 
> 1) You can't include vfw.h before windows.h, that breaks builds. Did you
> look at fate lately? It's kinda red-ish. Including these two in alphabetical
> order with respect to each other breaks things.
> 
> 2) Including windows.h before winsock2.h will give compiler warnings with
> mingw, and breaks the build with MSVC. libavformat/internal.h will include
> winsock2.h
> 
> Therefore you can't enforce the rule with system headers before local
> headers as long as windows.h is involved. Yes, it's of course ugly that
> their system headers aren't self-sufficient, but that's the way it is.

*sigh* - nothing is ever sane if it involves Windows.  Thanks for the
explanation.

Diego

Patch

diff --git a/libavdevice/vfwcap.c b/libavdevice/vfwcap.c
index 4182642..92d47ff 100644
--- a/libavdevice/vfwcap.c
+++ b/libavdevice/vfwcap.c
@@ -19,9 +19,6 @@ 
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
-#include <vfw.h>
-#include <windows.h>
-
 #include "libavutil/internal.h"
 #include "libavutil/log.h"
 #include "libavutil/opt.h"
@@ -30,6 +27,12 @@ 
 #include "libavformat/avformat.h"
 #include "libavformat/internal.h"
 
+// windows.h should be included after winsock2.h (if not, it may trigger
+// warnings but not errors), and libavformat internals may include winsock2.h
+#include <windows.h>
+// windows.h needs to be inclued before vfw.h
+#include <vfw.h>
+
 /* Some obsolete versions of MinGW32 before 4.0.0 lack this. */
 #ifndef HWND_MESSAGE
 #define HWND_MESSAGE ((HWND) -3)