eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

Introduce PathNameCS.sequenceOfNames() method back #431

Closed eclipse-ocl-bot closed 3 hours ago

eclipse-ocl-bot commented 3 hours ago

| --- | --- | | Bugzilla Link | 289111 | | Status | CLOSED WONTFIX | | Importance | P3 normal | | Reported | Sep 10, 2009 12:39 EDT | | Modified | May 27, 2011 02:47 EDT | | Reporter | Alex Shatalin |

Description

Recently this method was removed as a part of bug #287993.\ I suggest introducing it back. This will simplify implementation of OCL-based clients (like QVT/Xpand).\ if you think this method will introduce some garbage into an OCL AST, then just create utility class in a public package available for all the OCL clients and implement this method there.

eclipse-ocl-bot commented 3 hours ago

By Ed Willink on Sep 10, 2009 14:42

Re-introducing a derived getSequenceOfNames() is very easy.

AbstractOCLAnalyzer.createSequenceOfNames() does exactly what you require.

I'm puzzled by your comments about garbage in the OCL AST. This change should only affect the OCL CST. I would like to understand how this really affects you so that we can avoid other changes impacting more than is necessary.

If you can point at a couple of plug-ins to check, we can try to include them in a smoke test to see how much accidental damage we're causing.

eclipse-ocl-bot commented 3 hours ago

By Radomil Dvorak on Sep 10, 2009 15:40

(In reply to comment #1)\ I'm not quite convinced that replacing getSequenceOfNames() with getSimpleNames() was the best step. It seems that the only reason was to get offsets for individual name segments, which can easily be calculated from ITokens associated with PathNameCS.

Based on our experience with processing larger source files, I think it makes sense to keep memory consumption in mind and avoid too fine grained representation of pathnames.

eclipse-ocl-bot commented 3 hours ago

By Ed Willink on Sep 10, 2009 16:11

The change is not creating extra objects; the SimpleNameCS and IToken objects are created whether or not they are made visible in the PathNameCS.

The previous implementation of the

    pathNameCS '::' simpleNameCS

rule dismantled the SimpleNameCS and while

pathNameCS ::= IDENTIFIER

did not create a SimpleNameCS, this is associated with an ability to parse a variety of not reserved names such as MyClass::oclIsNew. The imminent fix for bugs 184048 and 288575 restructures the name parsing to make name usage intelligible. This identifies the inaccurate treatment of reserved names. However avoidance of shift reduce conflicts requires ALL names to go via SimpleNameCS and so there is no saving for the first pathNameCS contribution either.

If memory consumption is an issue, please raise a bugzilla to provide a facility to detach the AST from analyzer, parser, lexers, Environments, CST and ITokens so that they can be garbage collected.

eclipse-ocl-bot commented 3 hours ago

By Alex Shatalin on Sep 11, 2009 07:09

(In reply to comment #1)

AbstractOCLAnalyzer.createSequenceOfNames() does exactly what you require.

Ok. This method looks like a static utility one I was talking about, so it can be probably used from other QVT clients.

If you can point at a couple of plug-ins to check, we can try to include them in a smoke test to see how much accidental damage we're causing.

oe.m2m.qvt.oml was affected this time

I think you can have oe.m2m.qvt. checked out into your workspace and oe.gmf.xpand.

Should I close this bug as "invalid" and Radek QVT code will be adopted to use AbstractOCLAnalyzer.createSequenceOfNames()?

eclipse-ocl-bot commented 3 hours ago

By Ed Willink on Sep 11, 2009 13:50

I glanced at the qvt oml code and was initially inclined to put a derived getSequenceOfNames() in for you. But..

The change to getSimpleNames() allowed a vagueness in the CST usage to be cleared up. The List is sometimes modified to add/remove a head/tail. The PathNameCS is now immutable and a List is created by the caller for the caller to use at will.

If a derived getSequenceOfNames() used a cache, modifying practices could be dangerous.

If a derived getSequenceOfNames() creates a new copy each time, places where you invoke the method as many as five times are at least inefficient and possibly wrong.

It will be much better for you to invoke getSequenceOfNames() once and use the result many times.

I am very surprised that you are using the CST for more than parsing. Can you explain the use case for QvtOperationalVisitorCS. Is this an obsolete piece of code awaiting a rewrite, or is there some other utility for the CST that I/we need to understand?

[I am sorry that the invalid/OclInvalaid clarification hits you hard too.]

eclipse-ocl-bot commented 3 hours ago

By Ed Willink on May 27, 2011 02:47

Closing after over 18 months in resolved state.