Thanks for the feedback!
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"
This is easily possible.
* 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)
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.
* 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)
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
_______________________________________________
fpc-devel maillist - [email protected]
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
