On 31/10/15 11:33, Ondrej Pokorny wrote:
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
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
_______________________________________________
fpc-devel maillist - [email protected]
http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
