Hello,

just out of curiosity, I wanted to play with the debugger and try to add http://bugs.freepascal.org/view.php?id=28820

As the first step, I want to debug the compiler while it is compiling my simple project.

What I did:
1.) I opened "compiler\ppx86_64.lpi" from FPC trunk.

2.) I set run parameters to:
-MObjFPC -Scghi -O1 -g -gl -l -vewnhibq
-FiP:\programs\lazarus\_zk\CT_ClassConstruct\lib\x86-win64 -FuP:\programs\lazarus\_zk\CT_ClassConstruct\
-FUP:\programs\lazarus\_zk\CT_ClassConstruct\lib\x86-win64\ project1.lpr (basically the same Lazarus uses to build project1.lpr)

3.) I set working directory to:
p:\programs\lazarus\_zk\CT_ClassConstruct\
(directory with project1.lpr)

4.) I run it and get the following output:

Free Pascal Compiler version 3.1.1 [2015/10/30] for x86_64 Copyright (c) 1993-2015 by Florian Klaempfl and others (1002) Target OS: Win64 for x64
(3104) Compiling project1.lpr
Fatal: (10022) Can't find unit system used by project1

What could be wrong? Did I forget to supply the fpc.cfg or something like that?
Is there any wiki page about debugging the compiler?

Thanks
Ondrej

Got it working, wrong target directory!
Ondrej

Ondrej Pokorny wrote

just out of curiosity, I wanted to play with the debugger and try to add http://bugs.freepascal.org/view.php?id=28820

Seems like my little hint helped :P

I usually use -n to have the compiler not use any config file at all so I can be sure that it only uses the parameters I supply on the command line (this way of course I need to supply the RTL path and the paths for all other units I need as well). Also I have my test programs in a subdirectory of the SVN checkout (in my case that directory is called "fpctests") and the output directory next to it ("testoutput") with the working directory set to the checkout directory. This way I usually have run parameters like this:

-n -Furtl\units\i386-win32 -viwn -FEtestoutput .\fpctests\txyz.pp

This way I can also quite quickly check a test if I might have broken it, for example:

-n -Furtl\units\i386-win32 -viwn -FEtestoutput .\tests\test\thlp1.pp

Speaking of which: I suggest you to run the compiler test suite first without any modifications to the compiler and safe at least the faillist file. This way you can diff with testruns with your modifications to see if you broke something.

Like this (I assume current dir is the checkout directory):

make clean all FPMAKEOPT="-T 4"
cd tests
make clean full TEST_FPC=E:\path\to\checkout\compiler\ppc386.exe CHUNKSIZE=50 -j 4
mkdir output\unmodified
cp output\i386-win32\faillist output\unmodified
cd ..
{ do changes to compiler }
make clean all FPMAKEOPT="-T 4"
cd tests
make clean full TEST_FPC=E:\path\to\checkout\compiler\ppc386.exe CHUNKSIZE=50 -j 4
diff -aur output\unmodified\faillist output\i386-win32\faillist

(Alternatively to diff you can use a tool like WinMerge)

Regards,
Sven

Yes, indeed. It's always good to know that a patch will at least be considered for including into the trunk before taking the step and trying to write one. Otherwise it's wasted time ;)

Thanks for your tips! I uploaded a patch and commented on the issue report: http://bugs.freepascal.org/view.php?id=28820

Ondrej

Independent of whether it will be integrated, there are definitely a number things that should be changed first:
* don't use the "is" operator unless there is absolutely no other way to achieve the same effect. In the compiler, every node has a "nodetype" field that can be used to determine the kind instead (which is much faster than "is"
* I'm not sure such a "virtual node" is the best approach. I don't think we have that anywhere else in the compiler, so unless there is no cleaner way, we shouldn't introduce this concept (if some nodes are used differently from other nodes --other than the special error node--, then maintenance and reasoning becomes harder)
* don't add extra (semi-)global variables such as the tscannerfile.inforin field unless there is absolutely no other way to achieve the same effect. It's usually a quick hack, and there are way too many of those in the compiler already (I've removed a number of them over the years, but there are plenty left)
* like for all other node types, create a "tenumeratornodeclass = tclass of tenumeratornode" type, a "cenumeratornode: tenumeratornodeclass = tenumerator;" global variable and always use cenumeratornode.create() (so that the node class can be overridden by an architecture-specific class if necessary)
* it seems you have a memory leak (the tenumeratornode doesn't seem to be freed anywhere)

There may be more issues, but these are the ones that jump out at first sight.

Jonas

Thanks for the feedback!

This is easily possible.

I don't know a way to achieve it without these 2 points. Maybe I'll think of something in the future. For now, they seem to be necessary :( (But I don't know the compiler well, this was my first attempt to extend it, so I am not telling it is not possible. I just don't know any other way yet :)

Why I did it so:
inforin: the "enumerator" has to replace the property if and only if the evaluation is in the for-in statement. So you have to know that the property evaluation is done for the for-in-statement. If there is another way how to determine that the evaluation is done for the in-statement, it is possible to replace it (I don't know about it). tenumeratornode "virtual node": it is definitely the easiest way (maybe not the best) because the enumerator evaluation is done for the parent object and not for the enumerator function. Thanks to tenumeratornode, you can use the same compiler code for:

*for x in MyObj do**
*and
*for x in MyObj.Objects do**
*(*Objects = property **Objects[Index: xyz]: zyx read GetObject enumerator GetObjectEnumerator;)

*-> these statements are semantically equivalent, with the difference that with "MyObj.Objects" you choose an alternative *GetObjectEnumerator *enumerator function instead of *GetEnumerator*. The enumerator function and parent object are handed over with tenumeratornode.

This is easily possible.

* it seems you have a memory leak (the tenumeratornode doesn't seem to be freed anywhere)

No, there is no memory leak. tenumeratornode is destroyed in 'function for_in_loop_create(hloopvar: tnode): tnode;" in the last line: "expr.free;" - because expr is still the tenumeratornode - the expr parameter of create_for_in_loop is not var! You can easily check it by setting a breakpoint into tenumeratornode.destroy or run the compiler with heaptrc. There are some memoryleaks in ppc, but they were not introduced with the patch - there are the same memoryleaks with and without the patch.

Again, thanks for the feedback!

Ondrej

I have thought about the problem and I have some new ideas:

I did it and uploaded a new patch to the issue report.

The "virtual node" in the comment may be misleading. I changed the comment. I now think that actually the use of tenumeratornode is a pretty clean way to do it - it completely reflects the code usage. If I comment on the attached project1.lpr from the issue report:

The use of array property enumerator:
* for s in Self.StringArray do**
** Writeln(s);**
*
is the same as using the default enumerator (only with a different enumeration fuction):
* for s in Self do**
** Writeln(s);**
***
It means that internally you have to create the for-in loop with "create_enumerator_for_in_loop" with the same "expr" argument (pointing to Self) and with "pd" pointing to the actual enumeration function. tenumeratornode creates the link between "create_for_in_loop" and "handle_propertysym", which again reflects the syntax pretty well.

The argument that the tenumeratornode concept is completely different from other nodes is correct - but if you think about the syntax you will see that also the syntax is completely different from other pascal syntax so you need such a unique node. In the for-in case for array properties, the property is not evaluated as usual but the appropriate enumeration function and object have to be passed over to the for-in loop. tenumeratornode doesn't have any other meaning and cannot/must not be used anywhere else - so there shouldn't be any maintenance problems either.

If you know any other way how to detect that the compiler is evaluating the for-in section in the "handle_propertysym" procedure, I'll be happy to change it! I myself don't know it :(

Actually since tenumeratornode is pretty special and the only things used from tenumeratornode are the public variables *enumowner*, *enumproc* and *resultdef* - there is nothing to override. If you think the override concept is needed (I don't think so) then *enumowner*, *enumproc* should be moved to protected section and virtual getters *get**enumowner* and *getenumproc* should be introduced and used in "create_for_in_loop". I can do it but I don't see any usage for it. (I may be wrong here, so if you tell me I should do it, I'll do it.)

* it seems you have a memory leak (the tenumeratornode doesn't seem to be freed anywhere)

As I commented in my previous answer - there is no memory leak.

Again, thanks for the feedback!

Ondrej

Ondrej Pokorny wrote

The argument that the tenumeratornode concept is completely different from other nodes is correct - but if you think about the syntax you will see that also the syntax is completely different from other pascal syntax so you need such a unique node.

That is a very good argument not to add it...

Jonas

Yes, my argument that the "syntax is completely different from other pascal syntax" is misleading. I put it wrong. It is not different - its a new extension within the current pascal syntax:

E.g. the code
* for s in Self.StringArray do**
** Writeln(s);**
* would compile even now if *StringArray* was an "array of xyz" or a class/record/interface with GetEnumerator function. The syntax is already present in Pascal. I only want to extend it for array properties (this extension is new and unique).

The fact that the syntax is unique ensures that no legacy code can be broken. I personally don't think that adding many new compiler features is necessary, but this one really is useful - with this syntax you could easily and systematically add enumerators to a lot of different properties in the RTL/LCL you cannot use in a for-in loop now:
- TWinControl.Controls
- TStrings.Names
- TStrings.Objects
- TStrings.Values
- TStringGrid.Cols
- TStringGrid.Rows
- TStringGrid.Cells
- etc, etc

Michael liked it as well:
http://lists.freepascal.org/pipermail/fpc-devel/2015-October/036110.html

---

+ I was wrong about the need to use "tenumeratornode". Actually you can use "tcallnode" and get *pd* and *expr* for
*create_enumerator_for_in_loop* from its properties in function *create_for_in_loop*:
expr:=tcallnode(expr).methodpointer;
pd:=tprocdef(tcallnode(expr).symtableprocentry.ProcdefList[xyz]); // (you have to find the compatible overload)
(If you don't know what I mean I can send you the code. It's working like a charm.)

The only thing we need is a new flag/whatever so that *create_for_in_loop* knows that "tcallnode(expr)" contains an enumerator function. tnode.flags seems to be full :( Do you have any suggestions? If we sort this out, 2 more arguments against it are nil :)

Ondrej

The basic issue is that you need extra context for supporting this extension. Normally, an enumerator is searched based on the class type. In your case, it's searched either based on the class type, or if the class is returned from a property with an "enumerator" specifier, based on that enumerator declaration.

Whether you do it via a temporary node, a global variable and/or node flags, does not change that. Extra context is always bad both for the compiler implementation/maintenance and for code
readability/predictability by humans.

This leads to more design design decisions:
1) do you a) forbid an enumerator specification if the class already has an enumerator, b) hide the existing enumerator, or c) allow enumerating both via the property and the original class enumerator? If you all both, how do you differentiate? If b), first assigning the property to a variable and iterating over that will have a different result than directly iterating over the property, which is bad for understandability and code predictability. If c), how do you specify it. If a), that break orthogonality (and I'm sure some people will argue that iterating over a particular property may have to be done differently than over that class' contents in the general case)

2) do you expose the "hidden/parent" enumerator of the class itself to the overriding enumerator in a property and if so, how? Maybe even iterating both at the same time via some kind of inheritance mechanism?

There are undoubtedly more.

Jonas

Yes, the context is new. Yes, every new feature adds maintenance efforts. But this is not an argument...
Speaking from my point of view, I do expect to be able to enumerate over array properties just as I enumerate over array variables. It feels very natural.

I don't understand the design decisions you talk about below:

The extension is completely orthogonal - there is no redundancy in the code. Again if I take the project1.lpr from the issue report:

* for s in StringArray do**
** Writeln(s);**
*
will never compile without the extension. The StringArray property cannot introduce/return an enumerator. In absolutely no case.

2) do you expose the "hidden/parent" enumerator of the class itself to the overriding enumerator in a property and if so, how? Maybe even iterating both at the same time via some kind of inheritance mechanism?

If you are talking about something like this:

   TTest = class
   //...
   public
     function *GetEnumerator*: TTestObjectEnumerator;
     property StringArray[Index: Integer]: string read GetString
enumerator *GetEnumerator*;
   end;

then this is again absolutely no problem. The code:

* for s in Self do**
** Writeln(s);**
*
and

* for s in Self.StringArray do**
** Writeln(s);**
*
will do the same thing. Again, this is very natural.

There are undoubtedly more.

I don't think so. Please be specific. "undoubtedly more" doesn't sound very scientific :)

Ondrej

Jonas Maebe wrote

1) do you a) forbid an enumerator specification if the class already has an enumerator

No, it's not needed.

b) hide the existing enumerator

No, it's not needed.

or c) allow enumerating both via the property and the original class enumerator?

There can never be a situation where property and class enumerators compete. property enumerator is by design allowed only for array properties: *property A[index: xyz]*. They cannot introduce an enumerator with current trunk.

If you all both, how do you differentiate? If b), first assigning the property to a variable

You cannot assign an array property to a variable. It does not exist without the index parameter.

and iterating over that will have a different result than directly iterating over the property, which is bad for understandability and code predictability.

This can never happen. Array property does not exist without the index parameter.

---
You probably missed the fact that property enumerator can be introduced only for array properties. I talk about it in the issue report http://bugs.freepascal.org/view.php?id=28820 :
/
Please note that the "enumerator" keyword can be used only for "array" properties because normal properties can have a default enumerator defined./

Also the patch I uploaded is written in regard to this design. See in project1.lpr (uncomment the enumerator keyword for testing):
* property MyString: string read GetMyString;// enumerator GetEnumerator2; // <-- try to uncomment, this has to fail to compile (correct) !**
*

Ondrej

Ondrej Pokorny wrote

You cannot assign an array property to a variable. It does not exist without the index parameter.

OK, there is a bug in ppc that allows to use an array property with *any paramete**r(s)* (also no parameter). Delphi does not allow it.

Actually ppc doesn't check if *MyProp[Index]* matches the property declaration but if there is an (overloaded) function that matches the Index parameter. Therefore you can declare a parameterless function with property getter's name that returns the enumerator and it will be used in the for-in loop!

I still think that it is a ppc bug and it would be better to have a clean way to do that. But if you don't like to extend the compiler, please don't fix this bug either.

It was still a good practice for me to play with the compiler. I wouldn't have found the bug without studying the compiler code. You can find demo code that shows how to create an array property enumerator in the issue report: http://bugs.freepascal.org/view.php?id=28820#c87065

Sven Barth & Michael Van Canneyt: you also showed an interest. What is your opinion about it?

Ondrej

It's not a general compiler bug, but only in relationship with helpers (I can't trigger it if I have a class constructed that way without a helper). Nevertheless it's a bug and it should be fixed. As I have written the bug report: would you please report it as a separate bug?

I personally prefer the "enumerator" approach. You'll hear from me in a moment regarding your patch (I was a bit busy at the weekend otherwise I would have answered earlier).

Regards,
Sven

I personally think it's better to use tcallnode than to introduce another node. I know there is already a parser only node, namely specializen (added by me), but that node /really/ simplifies things unlike that enumerator node.

The only thing we need is a new flag/whatever so that *create_for_in_loop* knows that "tcallnode(expr)" contains an enumerator function. tnode.flags seems to be full :( Do you have any suggestions? If we sort this out, 2 more arguments against it are nil :)

I see three possibilities to avoid the addition of a global flag:
- simply always return the enumerator function if no arguments are given for an indexed array (pro: easy to implement; con: will bite us once we add another functionality that works on the array property as a whole)
- always return the enumerator, but add checks everywhere except the for-in parsing against the enumerator (maybe for this case the enumerator node would be an advantage) (pro: enumerator will only work in for-in; con: every expression handling code needs to maintain that code)
- extend comp_expr by another boolean parameter (which is set in for_in_loop_create) and pass that down to factor (even better: convert the boolean parameters of comp_expr, sub_expr and factor to a set, would be cleaner anyway, IMHO) (pro: the state is only maintained locally and new flags can be added easily; con: a greater change in the compiler, though that would be a onetime thing)

I personally would prefer point three. Maybe I'd even manage to do that change during this week (I have vacation after all, so I should have time ^^) and then you can redo your patch based on this.

Regards,
Sven

Sven Barth wrote

It's not a general compiler bug, but only in relationship with helpers (I can't trigger it if I have a class constructed that way without a helper).

It is a general compiler bug. Not even related to a for-in or anything else. See:

*program arraypropbug;**
**
**uses**
** sysutils;**
**
**type**
** TObj = class**
** public**
** function GetS(Index: Integer): string; overload;** ** function GetS(Index1, Index2: Integer): Double; overload;** ** property S[Index: Integer]: string read GetS;** ** end;**
**
**{ TObj }**
**
**function TObj.GetS(Index1, Index2: Integer): Double;** **begin**
** Result := Index1 / Index2;**
**end;**
**
**function TObj.GetS(Index: Integer): string;**
**begin**
** Result := IntToStr(Index);**
**end;**
**
**var**
** o: TObj;**
** d: Double;**
**begin**
** o := TObj.Create;**
** d := o.S[10, 3]; // !!!**
** Writeln(d);**
**end.**
*
Ondrej

I couldn't reproduce the previous example without arguments though (didn't have the idea to try /more/ parameters). But the one you just showed is troubling as well... would you please add that as an additional example to the bug report that you'll hopefully file for this problem? :)

Regards,
Sven

With pleasure :)

Yes it looks like if the parameter count matches, there is a compile error (correct). If it doesn't match (there are less or more parameters), it compiles (wrong).

Furthermore it doesn't only affect the getter but also the setter. All possible combinations compile. See attached project. I suspect the setter could be dangerous in connection to operator overloading.

Ondrej

http://bugs.freepascal.org/view.php?id=28949

Ondrej

I was just about to suggest the same.

I think your proposal regarding enumerators is good, and these bugs should definitely be fixed.

Michael.

First of all thanks Sven and Michael for the positive feedback! I was already worried that I was completely misunderstood with the property array enumerator extension.

About your 3 points, Sven:

Yes, this is not very clean. Furthermore such code would compile as well:

x := MyObj.MyArrayProperty;

(x would get the enumerator function, which is wrong.)

Looks unnecessarily complicated.

Looks unnecessarily complicated for me as well.

---

From my POV, having thought about the problem for some days already, the use of tenumeratornode is the simplest and clearest way to achieve the goal. The property array enumerator use in for-in is a "single case" only. So the use of a single-purpose node is clear as well - everybody understands on the first sight what is going on.

It is way more dangerous and unclear if you use some flags or parameters that change the tcallnode handling. If you use tenumeratornode you clearly see in what code parts it is used and how. If you use tcallnode
+ some flag/parameter it will be devil. For such reasons there was OOP invented, so we should take advantage of it.

If you don't want to include the "enumeratorn" into tnodetype and the "is" operator from my first proposal is too slow, you can also directly type check for tenumeratornode in create_for_in_loop (in the case you don't allow tenumeratornode ancestors, of course):

                 if *expr.ClassType = tenumeratornode* then
                   begin
                     // the expr is a property enumerator, use it directly
                     pd:=tenumeratornode(expr).enumproc;
                     expr:=tenumeratornode(expr).enumowner;
                   end

Ondrej

On 02.11.2015 20:24, Sven Barth wrote: >> >> The only thing we need is a new flag/whatever so that >> *create_for_in_loop* knows that "tcallnode(expr)" contains an

enumerator

for an indexed array (pro: easy to implement; con: will bite us once we add another functionality that works on the array property as a whole)

Not if we define it as the intended purpose. But also note that I had mentioned in my "con" that defining it like this might bite us layer on if we have a different usecase for code like the above. So I don't favor that option either anyway.

> - always return the enumerator, but add checks everywhere except the

for-in parsing against the enumerator (maybe for this case the enumerator node would be an advantage) (pro: enumerator will only work in for-in; con: every expression handling code needs to maintain that code)

Looks unnecessarily complicated.

That's my opinion as well ;)

> - extend comp_expr by another boolean parameter (which is set in

for_in_loop_create) and pass that down to factor (even better: convert the boolean parameters of comp_expr, sub_expr and factor to a set, would be cleaner anyway, IMHO) (pro: the state is only maintained locally and new flags can be added easily; con: a greater change in the compiler, though that would be a onetime thing)

Looks unnecessarily complicated for me as well.

And here we disagree. The "inforin" you added to tscannerfile is something we don't want to see introduced (especially since we're trying to reduce reliance on such (pseudo)global state) and the only way to fix this is by passing down the flag through comp_expr to handle_propertysym.

--- From my POV, having thought about the problem for some days already, the

use of tenumeratornode is the simplest and clearest way to achieve the goal. The property array enumerator use in for-in is a "single case" only. So the use of a single-purpose node is clear as well - everybody understands on the first sight what is going on.

No, I think using a tcallnode is a more powerful approach. create_for_in_loop only needs to check that the tcallnode returns a valid enumerator type (namely one containing "Current" and "MoveNext") and be done with it. This way one can easily extend the for-in with other functionality if the need arises.

It is way more dangerous and unclear if you use some flags or parameters

that change the tcallnode handling. If you use tenumeratornode you clearly see in what code parts it is used and how. If you use tcallnode + some flag/parameter it will be devil. For such reasons there was OOP invented, so we should take advantage of it.

I'm not talking about the handling of tcallnode I'm talking about when handle_propertysym is returning the enumerator or not (completely independent of the usage of tenumeratornode or tcallnode).

If you don't want to include the "enumeratorn" into tnodetype and the

"is" operator from my first proposal is too slow, you can also directly type check for tenumeratornode in create_for_in_loop (in the case you don't allow tenumeratornode ancestors, of course):

This has nothing to do with the discussion we're currently having. If a new node would be introduced it would have to be implemented exactly like it's done with the other nodes, namely using nodetype and potentially being extendable. There are reasons for this approach after all. However I'm still not of the opinion that the enumerator node is needed at all.

Regards,
Sven

Oh, here you are talking about the inforin global state. No, we don't disagree here. I am absolutely OK with replacing the inforin for a parameter in "handle_propertysym". But this doesn't have anything in common with the use of tenumeratornode. You can replace inforin with a different approach but it still doesn't mean that you automatically solved the problem with the use of tenumeratornode and you don't need it.

Note that you have to handle the enumerator property keyword in *handle_propertysym*. You have to return the enumerator function from palt_enumerator list here. (Issue 1)

The result then has to be correctly interpreted in *create_for_in_loop*. (Issue 2)

If we want to solve Issue 1 in a clean way (that means support property enumerator only in for-in), we need either a global state (inforin) or a new parameter for *handle_propertysym* and all the functions on top of it. I am absolutely OK with both variants.

If we want to solve Issue 2, we either need a new node (tenumeratornode) or a new varible/flag in tcallnode. It is not sufficient to know that we are in a for-in loop. We have to know that the returned node from *create_for_in_loop* really is the enumerator to use.

This is not save. This approach could interfere with current code. What about if your object contains "Current" and "MoveNext" but is not the actual enumerator to use? If you use for-in on a
variable/function/whatever that returns such an object, the object "Self" will be used as the enumerator and not the "Self.GetEnumerator". As a result you get wrong enumerator object and, furthermore, your object gets destroyed after the for-in loop because enumerator objects are automatically freed. I can write you a sample code to prove it.

=> Don't do it :)

I am completely for this! It was just an idea if you /should//not/ want to introduce a new node. If you are not principally against having a complete new node, forget the idea :)

Ondrej

I see that Sven refactored the compiler and added texprflags. Thanks Sven! I uploaded a new patch that takes advantage of texprflags.

/property-enumerator-4.patch/

Jonas Maebe wrote

* don't use the "is" operator unless there is absolutely no other way to achieve the same effect. In the compiler, every node has a "nodetype" field that can be used to determine the kind instead (which is much faster than "is"

Fixed with "enumeratorn" nodetype.

Nothing changed in this respect. I still use tenumeratornode.

Fixed with texprflags and forin parameter.

Implemented.

Ondrej