On Thu, August 4, 2011 22:32, Marcus Sackrow wrote: > Diff (on top of 18076) for new Target: AROS (i386) > tried to keep the diff as short as possible but its still 750 K, > so I placed it on my Webpage: > > http://www.alb42.de/AROS-Target.diff > > Compiler, RTL and Package: arosunits > > I hope its ok in this way
Thanks for your contribution. While other core team members are involved in reviewing your patch, I have some questions related to your FExpand modifications. However, even before that, there's one more thing I'm quite clear about (sorry if this was already discussed somewhere and I just missed it): Your diff contains patches to files in /rtl/aros/, but such files obviously do not exist in our SVN right now. Shall I assume that those should be based on a copy of /rtl/amiga/ (as suggested by the patch for system.pp), or a copy of /rtl/morphos/ (as suggested by the patch for dos.pp), or what is the base of your diff? Now to the concrete points: - I see that your sysutils.pp patch adds a new conditional define for FExpand. _If_ we need to add something like that, it should be added to the list documented explicitly in comments at the end of fexpand.inc in order to have a complete list in one place. However, your patch for sysutils.pp assumes that both DirectorySeparator and DriveSeparator may be used as markers for root directory. Is this really the case? Could you provide some examples (possible file paths as used in AROS) to show how this is used? Could you possibly suggest extensions to /tests/test/units/dos/tfexpand.pp to test the proper behaviour of FExpand under AROS? In addition, I've noticed that you added the new define only to sysutils.pp, but not dos.pp; fexpand.inc is used in both and the defines should be consistent. - It would be nice if you can add some comments clarifying the reason of your changes (for incorporation into the code). FExpand implementation is quite complex and it's useful to keep the comments there, otherwise it becomes unmaintainable. - I noticed that your patch to fexpand.inc contains explicit references to '/' and ':' characters. This is wrong, you should always refer to constants like DriveSeparator, DirectorySeparator, AllowDirectorySeparators, etc. If the existing ones are not suitable for your need, we should either discuss how to make it more general (preferably) or to put it under a platform specific IFDEF if there really isn't a better choice. Regards Tomas _______________________________________________ fpc-devel maillist - [email protected] http://lists.freepascal.org/mailman/listinfo/fpc-devel
