[fpc-devel] New FPC Target: AROS

[Tomas Hajny]

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