Bug 337937

Comment 1 Emanuel Graf 2011-02-23 03:02:07 EST

Created attachment 189578 [details]
Implementation of the proposed change

Comment 2 Marc-André Laperle 2011-02-23 08:05:22 EST

Hi Emanuel. Thank you for working on this.
-The year in copyrights should be 2011
-Some documentation in IASTCopyLocation would be good

Comment 3 Emanuel Graf 2011-02-23 08:09:43 EST

(In reply to comment #2)
> Hi Emanuel. Thank you for working on this.
> -The year in copyrights should be 2011
> -Some documentation in IASTCopyLocation would be good

Thanks I noticed this too, I'm working on this right now.

Comment 4 Markus Schorn 2011-02-23 08:35:00 EST

Hmm, exposing the IASTNodeLoactions from the AST in a copy introduces new
restrictions on how (long) these copies can be used. Striclty speaking this
is a breaking API change, that can potentially cause ugly failures with
clients using this API. This needs to be considered, at least it needs to
be documented.

The restriction is, that an AST may only be used in one thread and must be
released, when the read-lock of the index that was used for creating the AST is
given up. This restriction would extend to all nodes obtained via copy()
from any of the nodes of the AST.

Comment 5 Mike Kucera 2011-02-23 09:02:56 EST

Perhaps we could make copying the locations optional. Leave the old API in place and add a new method:

IASTNode.copyWithLocations()
or
IASTNode.copy(boolean copyLocations)

And of course fully document the restrictions in Javadoc.

Comment 6 Markus Schorn 2011-02-23 09:13:31 EST

(In reply to comment #5)
> Perhaps we could make copying the locations optional. Leave the old API in
> place and add a new method:
> IASTNode.copyWithLocations()
> or
> IASTNode.copy(boolean copyLocations)
> And of course fully document the restrictions in Javadoc.

I support this idea (either version), it's a clean solution.

Comment 7 Emanuel Graf 2011-02-23 09:29:57 EST

(In reply to comment #5)
> Perhaps we could make copying the locations optional. Leave the old API in
> place and add a new method:
> 
> IASTNode.copyWithLocations()
> or
> IASTNode.copy(boolean copyLocations)
> 
> And of course fully document the restrictions in Javadoc.

I would vote for ASTNode.copyWithLocations()

Comment 8 Emanuel Graf 2011-03-03 07:08:46 EST

Created attachment 190251 [details]
new implementation with additional method

I still prefer ASTNode.copyWithLocations()but a clean implementation is not possible without exposing the necessary internal method in the interfaces. Therefore I choose the option with a parameter but replaced the boolean parameter with an enum. Because node.copy(CopyStyle.withLocations) communicates better what is happening then node.copy(true).

Comment 9 Emanuel Graf 2011-03-07 02:27:19 EST

Fixed in HEAD > 20110307

Comment 10 Marc-André Laperle 2011-03-08 00:40:51 EST

Created attachment 190613 [details]
UPC and XLC compile errors patch

Emanuel, you missed some changed in UPC and XLC. I made the changes and ran the tests. Can you still review the patch to make sure? It looks pretty safe so I will commit to HEAD and hopefully it will fix the build.

Comment 11 Marc-André Laperle 2011-03-08 00:43:54 EST

Last patch committed to HEAD > 20110308

Comment 12 CDT Genie 2011-03-08 01:23:26 EST

*** cdt cvs genie on behalf of mlaperle ***
Bug 337937 - CopyLocation for copied AST-Node. Fix UPC and XLC compile errors

[*] XlcCPPASTVectorTypeSpecifier.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/xlc/org.eclipse.cdt.core.lrparser.xlc/parser/org/eclipse/cdt/internal/core/lrparser/xlc/ast/XlcCPPASTVectorTypeSpecifier.java?root=Tools_Project&r1=1.2&r2=1.3
[*] XlcCASTVectorTypeSpecifier.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/xlc/org.eclipse.cdt.core.lrparser.xlc/parser/org/eclipse/cdt/internal/core/lrparser/xlc/ast/XlcCASTVectorTypeSpecifier.java?root=Tools_Project&r1=1.2&r2=1.3

[*] UPCASTSimpleDeclSpecifier.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTSimpleDeclSpecifier.java?root=Tools_Project&r1=1.4&r2=1.5
[*] UPCASTTypeIdSizeofExpression.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTTypeIdSizeofExpression.java?root=Tools_Project&r1=1.2&r2=1.3
[*] UPCASTForallStatement.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTForallStatement.java?root=Tools_Project&r1=1.4&r2=1.5
[*] UPCASTCompositeTypeSpecifier.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTCompositeTypeSpecifier.java?root=Tools_Project&r1=1.4&r2=1.5
[*] UPCASTTypedefNameSpecifier.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTTypedefNameSpecifier.java?root=Tools_Project&r1=1.5&r2=1.6
[*] UPCASTSynchronizationStatement.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTSynchronizationStatement.java?root=Tools_Project&r1=1.5&r2=1.6
[*] UPCASTKeywordExpression.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTKeywordExpression.java?root=Tools_Project&r1=1.8&r2=1.9
[*] UPCASTLayoutQualifier.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTLayoutQualifier.java?root=Tools_Project&r1=1.3&r2=1.4
[*] UPCASTUnarySizeofExpression.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTUnarySizeofExpression.java?root=Tools_Project&r1=1.2&r2=1.3
[*] UPCASTEnumerationSpecifier.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTEnumerationSpecifier.java?root=Tools_Project&r1=1.4&r2=1.5
[*] UPCASTElaboratedTypeSpecifier.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/upc/org.eclipse.cdt.core.parser.upc/src/org/eclipse/cdt/internal/core/dom/parser/upc/ast/UPCASTElaboratedTypeSpecifier.java?root=Tools_Project&r1=1.4&r2=1.5

Comment 13 Emanuel Graf 2011-03-08 01:51:39 EST

Comment on attachment 190613 [details]
UPC and XLC compile errors patch

Patch look good, sorry that i missed those two plugins.