Re: [AD] include path fixup patch.

[Peter Wang <novalazy@xxxxxxxxxx>]

Review follows.

> Index: cmake/FileList.cmake
> ===================================================================
> --- cmake/FileList.cmake	(revision 9797)
> +++ cmake/FileList.cmake	(working copy)
> @@ -442,7 +442,7 @@
>      )
>  
>  set(ALLEGRO_INCLUDE_FILES
> -    include/allegro5/allegro5.h
> +    include/allegro5/allegro.h
>      include/allegro5/allegro.h
>      include/allegro5/winalleg.h
>      include/allegro5/xalleg.h

That's wrong.

> Index: demo/include/a5teroids.hpp
> ===================================================================
> --- demo/include/a5teroids.hpp	(revision 9797)
> +++ demo/include/a5teroids.hpp	(working copy)
> @@ -1,7 +1,7 @@
>  #ifndef A5TEROIDS_HPP
>  #define A5TEROIDS_HPP
>  
> -#include "allegro5/allegro5.h"
> +#include "allegro5/allegro.h"
>  #include "a5_font.h"
>  
>  #ifdef __linux__

What's the reason for this change, here and elsewhere?

> Index: src/xglx/xglx.h
> ===================================================================
> --- src/xglx/xglx.h	(revision 9797)
> +++ src/xglx/xglx.h	(working copy)
> @@ -6,7 +6,7 @@
>  #include <string.h>
>  #include <stdio.h>
>  
> -#include "allegro5/allegro5.h"
> +#include "allegro5/allegro.h"
>  #include "allegro5/internal/aintern.h"
>  #include "allegro5/platform/aintunix.h"
>  #include "allegro5/internal/aintern_system.h"

Ditto.


> Index: src/allegro.c
> ===================================================================
> --- src/allegro.c	(revision 9797)
> +++ src/allegro.c	(working copy)
> @@ -20,7 +20,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  
> -#include "allegro5/allegro.h"
> +#include "allegro5/allegro5.h"
>  #include "allegro5/internal/aintern.h"
>  #include "allegro5/internal/aintern_dtor.h"
>  

> Index: src/macosx/main.m
> ===================================================================
> --- src/macosx/main.m	(revision 9797)
> +++ src/macosx/main.m	(working copy)
> @@ -16,7 +16,7 @@
>   */
>  
>  
> -#include "allegro5.h"
> +#include "allegro5/allegro.h"
>  #include "allegro5/internal/aintern.h"
>  #include "allegro5/platform/aintosx.h"
>  

Which header is going to be used internally, allegro5/allegro.h or
allegro5/allegro5.h?  The patch is inconsistent, or at least I can't see a
pattern.

> Index: docs/src/ahack._tx
> ===================================================================
> --- docs/src/ahack._tx	(revision 9797)
> +++ docs/src/ahack._tx	(working copy)
> @@ -189,11 +189,11 @@
>  Header Files
>  
>  allegro.h lives in the include/ directory. It is only a placeholder which
> -includes other headers which live in the include/allegro/ tree. The reason
> +includes other headers which live in the include/allegro5/ tree. The reason
>  for this slightly odd approach is that allegro.h can include things like
>  "allegro/keyboard.h", which will work both in-situ within the build
>  directory, and if we copy allegro.h to the system include directory and the
> -other headers into system_include/allegro/. This avoids cluttering the
> +other headers into system_include/allegro5/. This avoids cluttering the
>  system directories with lots of our headers, while still allowing programs
>  to just #include &ltallegro.h&gt, and also makes it possible for people to
>  access keyboard stuff with #include &ltallegro/keyboard.h&gt.

This paragraph is out of date (just pointing it out, it's not a problem of your
patch).

> Index: docs/src/changes._tx
> ===================================================================
> --- docs/src/changes._tx	(revision 9797)
> +++ docs/src/changes._tx	(working copy)
> @@ -299,7 +299,7 @@
>     Trent Gamblin renamed scroll_display to scroll_screen in display.c
>  <li>
>     Trent Gamblin Removed commented code from src/display.c. Removed
> -   include/allegro/display.h and src/compat/cogfx.c
> +   include/allegro5/display.h and src/compat/cogfx.c
>  <li>
>     Elias Pschernig renamed XDUMMY driver to XGLX.
>  <li>
> @@ -3438,7 +3438,7 @@
>     for MinGW32 consistent with other Windows ports.
>  <li>
>     3.9.40: Henrik Stokseth improved make install to install only the needed
> -   headers from the include/allegro/platform directory.
> +   headers from the include/allegro5/platform directory.
>  <li>
>     3.9.40: Vincent Penquerc'h updated 7 makefiles after his header splitting
>     patch.

changes._tx shouldn't be updated.

> Index: makefile.lst
> ===================================================================
> --- makefile.lst	(revision 9797)
> +++ makefile.lst	(working copy)
> @@ -3,6 +3,8 @@
>  ALLEGRO_SRC_FILES = \
>  	src/allegro.c \
>  	src/blit.c \
> +	src/blenders.c \
> +	src/bitmap_new.c \
>  	src/bmp.c \
>  	src/clip3d.c \
>  	src/clip3df.c \
> @@ -14,7 +16,8 @@
>  	src/digmid.c \
>  	src/dither.c \
>  	src/dispsw.c \
> -        src/display.c \
> +	src/display.c \
> +	src/display_new.c \
>  	src/dtor.c \
>  	src/drvlist.c \
>  	src/events.c \
> @@ -49,6 +52,7 @@
>  	src/modesel.c \
>  	src/mousenu.c \
>  	src/pcx.c \
> +	src/pixels.c \
>  	src/poly3d.c \
>  	src/polygon.c \
>  	src/quantize.c \
> @@ -76,7 +80,14 @@
>  	src/compat/cokeybd.c \
>  	src/compat/comouse.c \
>  	src/compat/cotimer.c \
> -	src/misc/vector.c
> +	src/misc/vector.c \
> +	src/memblit1.c \
> +	src/memblit.c \
> +	src/memblit2.c \
> +	src/memdraw.c \
> +	src/system_new.c \
> +	src/memblit3.c \
> +	src/convert.c
>  
>  ALLEGRO_SRC_C_FILES = \
>  	src/c/cblit16.c \
> @@ -350,6 +361,15 @@
>  	src/x/xdga2s.s \
>  	src/x/xwins.s \
>  	src/misc/colconv.c
> +
> +ALLEGRO_SRC_XGLX_FILES = \
> +	src/xglx/xbitmap.c \
> +	src/xglx/xcompat.c \
> +	src/xglx/xdisplay.c \
> +	src/xglx/xdraw.c \
> +	src/xglx/xfullscreen.c \
> +	src/xglx/xglx_config.c \
> +	src/xglx/xsystem.c
>  	
>  ALLEGRO_SRC_QNX_FILES = \
>  	src/qnx/qdrivers.c \
> @@ -545,7 +565,6 @@
>  	examples/exmem.c \
>  	examples/exmidi.c \
>  	examples/exmouse.c \
> -	examples/exnew_events.c \
>  	examples/expackf.c \
>  	examples/expal.c \
>  	examples/expat.c \
> @@ -568,7 +587,16 @@
>  	examples/exunicod.c \
>  	examples/exupdate.c \
>  	examples/exxfade.c \
> -	examples/exzbuf.c
> +	examples/exzbuf.c \
> +	examples/exnewapi.c \
> +	examples/exnew_bitmap.c \
> +	examples/exnew_fs_resize.c \
> +	examples/exnew_icon.c \
> +	examples/exnew_lockbitmap.c \
> +	examples/exnew_lockscreen.c \
> +	examples/exnew_mouse.c \
> +	examples/exnew_mouse_events.c \
> +	examples/exnew_resize.c

Putting exnew* at the back is okay but it's not as simple as just sorting the
whole list and might make it harder to compare two lists (e.g. with the output
of ls).

>  
>  ALLEGRO_EXAMPLE_EXES = \
>  	examples/ex12bit \
> @@ -597,7 +625,6 @@
>  	examples/exmem \
>  	examples/exmidi \
>  	examples/exmouse \
> -	examples/exnew_events \
>  	examples/expackf \
>  	examples/expal \
>  	examples/expat \
> @@ -620,7 +647,16 @@
>  	examples/exunicod \
>  	examples/exupdate \
>  	examples/exxfade \
> -	examples/exzbuf
> +	examples/exzbuf \
> +   examples/exnewapi \
> +   examples/exnew_bitmap \
> +   examples/exnew_fs_resize \
> +   examples/exnew_icon \
> +   examples/exnew_lockbitmap \
> +   examples/exnew_lockscreen \
> +   examples/exnew_mouse \
> +   examples/exnew_mouse_events \
> +   examples/exnew_resize

Those should be hard tabs to be consistent with the rest of the file.

> Index: misc/grabdeps.pl
> ===================================================================
> --- misc/grabdeps.pl	(revision 0)
> +++ misc/grabdeps.pl	(revision 0)
> @@ -0,0 +1,40 @@
> +#!/usr/bin/perl

A short synopsis here would be nice and mention where it's called from.

> +
> +use warnings;
> +use strict;
> +use File::Spec;
> +
> +# we assume we are running from the allegro root dir
> +my $inc_dir = 'include';
> +my $file = $ARGV[0];
> +our %deps;
> +
> +grab_deps($file);
> +
> +print " ", join(" ", keys %deps);
> +
> +sub grab_deps {
> +	my $gpath = shift();
> +	open my $fh, $gpath or return; #die "failed to open $path: $!";
> +	for my $line (<$fh>) {
> +		if($line =~ /#\s*include\s+"([^"]+)"/) {
> +			my $file = $1;
> +			my $path = File::Spec->catfile($inc_dir, $file);
> +			if($file =~ /\.s$/ || $file =~ /\.inc$/) {
> +				my ($volume,$dirs,$file) = File::Spec->splitpath( $gpath );
> +				my $rpath = File::Spec->catfile($dirs, $file);
> +				$deps{$rpath} = 1;
> +
> +				grab_deps($rpath);
> +			}
> +			elsif(-f $file || $file =~ /asmdef/) {
> +				$deps{$file} = 1;
> +				grab_deps($file);
> +			}
> +			elsif(!exists $deps{$path} && -f $path) {
> +				$deps{$path} = 1;
> +				grab_deps($path);
> +			}
> +		}
> +	}
> +}
> 
> Property changes on: misc/grabdeps.pl
> ___________________________________________________________________
> Name: svn:executable
>    + *
> 

> Index: misc/deplib.sh
> ===================================================================
> --- misc/deplib.sh	(revision 9797)
> +++ misc/deplib.sh	(working copy)

I didn't review this file.  I'll assume it's okay ;)

...


> Index: misc/checklst.pl
> ===================================================================
> --- misc/checklst.pl	(revision 0)
> +++ misc/checklst.pl	(revision 0)
> @@ -0,0 +1,56 @@
> +#!/usr/bin/perl

Description.

> +
> +use strict;
> +use warnings;
> +use File::Spec::Functions;
> +
> +my $path = $ARGV[0];
> +
> +if(!defined $path || !-d $path) {
> +	print "usage $0 <path to allegro root>\n";
> +	exit 0;
> +}
> +
> +my %main_sources;
> +my $srcdir = catdir $path, "src";
> +opendir DIR, $srcdir or die "failed to scan $path: $!\n";
> +for my $ent (readdir DIR) {
> +	my $fpath = catfile('src',$ent);
> +	next if !-f $fpath;
> +	if($ent =~ /\.c$/) {
> +		$main_sources{$fpath} = 1;
> +	}
> +}
> +closedir DIR;
> +
> +my $lstdata;
> +open FH, catfile($path,"makefile.lst") or die "failed to open makefile.lst: $!";
> +	$lstdata = join '', <FH>;
> +close FH;
> +
> +$lstdata =~ s/\\\r?\n//sg;
> +$lstdata =~ s/[ \t]+/ /g;
> +my @lines = split /\r?\n/, $lstdata;
> +
> +my %lstdata;
> +for my $line (@lines) {
> +	chomp $line;
> +	next if $line =~ /^#/;
> +	next if $line =~ /^\s*$/;
> +	my ($name, $value) = split /\s*=\s*/, $line;
> +	$lstdata{$name} = $value;
> +#	print $name."\n";
> +}
> +
> +#print $lstdata{"ALLEGRO_SRC_FILES"}."\n";
> +
> +my %srcfiles = map { $_ => 1 } split /\s/, $lstdata{"ALLEGRO_SRC_FILES"};
> +my @missing_sources;
> +for my $file (keys %main_sources) {
> +	if(!exists $srcfiles{$file}) {
> +#		print "warning, $file exists on disk, but wasn't added to ALLEGRO_SRC_FILES\n";
> +		push @missing_sources, $file;
> +	}
> +}
> +
> +print "warning, the following files exist, but aren't in ALLEGRO_SRC_FILES: ".join(" ",@missing_sources)."\n";
> 
> Property changes on: misc/checklst.pl
> ___________________________________________________________________
> Name: svn:executable
>    + *
> 

> Index: examples/exswitch.c
> ===================================================================
> --- examples/exswitch.c	(revision 9797)
> +++ examples/exswitch.c	(working copy)
> @@ -17,7 +17,7 @@
>  
>  #include <math.h>
>  
> -#include <allegro5.h>
> +#include <allegro5/allegro5.h>
>  
>  
>  

Maybe the old examples should use allegro.h, and the new ones use allegro5.h?
The idea being that, one day, A4 compatibility stuff will be visible if you
include allegro.h, but not allegro5.h.

> Index: examples/exconfig.c
> ===================================================================
> --- examples/exconfig.c	(revision 9797)
> +++ examples/exconfig.c	(working copy)
> @@ -12,7 +12,7 @@
>   */
>  
>  
> -#include <allegro5.h>
> +#include <allegro5/allegro5.h>
>  
>  
>  int main(void)

Ditto for the other examples.

Peter