Re: [AD] include path fixup patch.

[Thomas Fjellstrom <tfjellstrom@xxxxxxxxxx>]

On Tue January 8 2008, Peter Wang wrote:
> 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
>
>
> -------------------------------------------------------------------------
> Check out the new SourceForge.net Marketplace.
> It's the best place to buy or sell services for
> just about anything Open Source.
> http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplac
>e


Ah, yeah, I thought I had "re fixed" the allegro.h problem. hmm. See, I used 
grep and sed to do those changes, and tried a couple things till 
autoconf/make didn't throw up. Some files that shouldn't have gotten touched 
did, I forgot about not touching changes._tx, oops. I'll revert that one.

Ok, that's all more than reasonable, I'll put together a new patch.

-- 
Thomas Fjellstrom
tfjellstrom@xxxxxxxxxx