Sign in Join

Live chat for Free Pascal, Lazarus, Delphi and every Pascal programmer.

Mailing lists / fpc-devel / 2011-08

New FPC Target: AROS

10 messages · 5 people · –

Reply by email Subscribe to this list Your subscription

Read-only archive. Use j and k to move between messages.

Tomas Hajny
to Marcus Sackrow
Marcus Sackrow wrote 9 lines
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

Reply by email

Marcus Sackrow
to Tomas Hajny
Tomas Hajny wrote 19 lines
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?

Its my first time using diff export from svn and I didn't notice it does not show the copy as well
the base was /rtl/amiga,, just use some copy paste of text from morphos.

Show quoted text 18 lines
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.

perhaps I didnt choose the name for the define well in fact I'm a little bit in trouble here because normaly at amiga /xxx/ would be in something like ../xxx/ (in Linux) and so its implemented in FExpand atm,
but the make util (and much other tools for development) in Aros always give pathes like /System/Development/ .... which should be resolved to System:Development/
in principle both cases are not distiguishable
to get fpc self compileable at Aros I had to add this function to FExpand... I'm open to other ideas.

Show quoted text 7 lines
- 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.

of course this can be changed

Greets
Marcus

Reply by email

Karoly Balogh
to Marcus Sackrow

Hi,

Marcus Sackrow wrote 10 lines
perhaps I didnt choose the name for the define well in fact I'm a little bit in trouble here because normaly at amiga /xxx/ would be in something like ../xxx/ (in Linux) and so its implemented in FExpand atm, but the make util (and much other tools for development) in Aros always give pathes like /System/Development/ .... which should be resolved to System:Development/ in principle both cases are not distiguishable to get fpc self compileable at Aros I had to add this function to FExpand... I'm open to other ideas.

This is because GNU make port is based on ixemul, which is a POSIX layer on top of Amiga-like systems, so it uses POSIX paths, while AmigaDOS under the non-ixemul FPC port expect Amiga-style paths.

In general, the compiler should work with Amiga paths (like on MorphOS), but accept and output Un*x paths, where GNU tools expect it. On MorphOS it has a platform specific PathConv() function in the RTL, which is triggered at the required places. Since this is highly Amiga/ixemul specific, and the mixture of two different path concepts, it should be kept outside of /rtl/inc, IMO.

Though I'm not really familiar with AROS, so there could be caveats there, I'm unaware of. :) If something is missing, it can still be added to a platform-specific extension.

The problem with your example in particular that /System/Development as Amiga path is perfectly valid, as it means ../System/Development in POSIX path, not System:Development/ Having this in FExpand() affects all apps using the RTL on AROS, and can end with tons of random side effects.

Greetings,

Charlie

Reply by email

Karoly Balogh
to Karoly Balogh

Hi,

Karoly Balogh wrote
The problem with your example in particular that /System/Development as Amiga path is perfectly valid, as it means ../System/Development in POSIX path, not System:Development/ Having this in FExpand() affects all apps using the RTL on AROS, and can end with tons of random side effects.

OK, lets clarify this. /System/Development can be interpreted both as Amiga path and Un*x path. As Un*x path it indeed can be translated to Amiga path System:Development/, but if it's an Amiga path, it can be translated to ../System/Development in Un*x. You simply cannot tell by parsing simply the input of FExpand inside FExpand, that what you should do there, as both are valid options.

This is why a separate function is need to be added, where the context (the calling environment) requires the path conversion.

Charlie

Reply by email

Tomas Hajny
to Karoly Balogh
Karoly Balogh wrote 19 lines
Hi, On Fri, 5 Aug 2011, Karoly Balogh wrote: > The problem with your example in particular that /System/Development as > Amiga path is perfectly valid, as it means ../System/Development in > POSIX > path, not System:Development/ Having this in FExpand() affects all apps > using the RTL on AROS, and can end with tons of random side effects. OK, lets clarify this. /System/Development can be interpreted both as Amiga path and Un*x path. As Un*x path it indeed can be translated to Amiga path System:Development/, but if it's an Amiga path, it can be translated to ../System/Development in Un*x. You simply cannot tell by parsing simply the input of FExpand inside FExpand, that what you should do there, as both are valid options. This is why a separate function is need to be added, where the context (the calling environment) requires the path conversion.

OK, I see. The current FExpand implementation might allow even including it twice with two different contexts / conditional defines (although not within the same namespece/unit) if that is useful for your needs. The other option is obviously always converting the paths to the native format (and possibly back) in case of invocation from the "foreign" context as suggested by you. Native Amiga paths should be hopefully supported well by the existing FExpand implementation. However, if you find any issues, please provide the appropriate test cases (preferably as a diff to tfexpand.pp, but pairs input_path / expected_output together with description of the starting situation regarding the current directory, etc., are OK too) and I'll be happy to fix it.

Tomas

Reply by email