The static constants AbstractTermFactory.EMPTY_LIST and TermFactory.EMPTY_LIST should be declared private because their use potentially contradicts the value of defaultStorageType that would be used when calling TermFactory.makeList(). In particular, building an application with an EMPTY_LIST argument fails in ParentTermFactory because no attachment can be registered to EMPTY_LIST.

Submitted by Sebastian Erdweg on 18 March 2011 at 11:47

On 18 March 2011 at 12:04 Lennart Kats commented:

Good point. I think it should be marked as deprecated. Internally, a different private field could be used. For other classes, a call to makeList() should be used instead.

On 18 March 2011 at 12:06 Sebastian Erdweg commented:

I am actually experiencing bugs because of this issue, thus I would prefer a more invasive fix then depreciation only.

On 18 March 2011 at 12:25 Lennart Kats commented:

I’m always hesitant to just break binary compatibility like that. The real problem is with how EMPTY_LIST is used now. I think all those uses should be reconsidered. Many of them are just harmless comparisons, intended to help performance a bit, they could use some kind of package-internal SHARED_EMPTY_LIST field instead. The cases where EMPTY_LIST is actually returned are harmful. One particularly problematic case is probably StrategoTerm.getAnnotations(). There doesn’t seem to be a nice way to fix that one; it really should be shared. If a term t has an empty list of annotations t’, then what are the annotations of list t’?

Perhaps ParentTermFactory and OriginTermFactory should just handle EMPTY_LIST as a special case, and create a new, unshared object for it? Which also wouldn’t be very nice…

On 18 March 2011 at 12:30 Sebastian Erdweg commented:

Wouldn’t it also make sense to just generate new non-shared term whenever an attachment is registered to a shared term. Then you have lazy duplication of terms and the existing code is untouched.

Apart from that I agree that the uses of EMPTY_LIST need to be reconsidered.

On 18 March 2011 at 12:38 Lennart Kats commented:

For parent terms it would be prohibitively expensive to clone terms in the general case. Trees are constructed in a bottom-up fashion, that would be a lot of recursive cloning operations of increasingly larger trees. The parent term factory is only used inside the parser, or if you happen to use the SSL_EXT_clone_and_set_parents primitive, so mutably setting parents should be safe as the subterms can never escape before the tree is fully constructed.

Origins are a different story: in Spoofax, the OriginTermFactory is always active. For the OriginTermFactory I don’t think it would be a problem to clone small subtrees like [] though, so perhaps that factory should just have a getStorageType() test to see if it should clone a tree before adding the attachment.

On 21 March 2011 at 10:57 Lennart Kats tagged 1.0

On 23 November 2011 at 17:57 Lennart Kats tagged 1.5

On 23 November 2011 at 17:57 Lennart Kats removed tag 1.0

On 8 January 2013 at 12:55 Eelco Visser removed tag 1.5

On 8 January 2013 at 12:55 Eelco Visser tagged interesting

On 8 January 2013 at 15:05 Eelco Visser tagged 1.1

On 8 January 2013 at 17:39 Gabriël Konat removed tag interesting

On 22 January 2013 at 08:51 Gabriël Konat tagged performance

On 30 January 2013 at 13:24 Vlad Vergu tagged 1.2

On 30 January 2013 at 13:24 Vlad Vergu removed tag 1.1

On 13 March 2014 at 08:52 Eelco Visser removed tag 1.2

Log in to post comments