[fpc-devel] Debug compiler

[Ondrej Pokorny]

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

On 31.10.2015 12:06, Jonas Maebe wrote:
> 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 did it and uploaded a new patch to the issue report.

> * 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)

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.

> * 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)

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 :(

> * 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)

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freepascal.org/pipermail/fpc-devel/attachments/20151031/b07ad7cb/attachment.html>