PsiElement.copy()

Eugene Vigdorchik (JetBrains) wrote:

So what I want to do in 5.0.3 (not in 5.0.2) is to copy only the subtree of
the element. The context of the copy would be the old element's parent,

Ok, I would have never expected that the copied element's parent(s) are also copied. Seems
a bit weird actually.

However it becomes illegal to examine the psi of the copy parents with e.g. getContainingClass(),
getContainingFile() or getParent(). The code that needs this has to
explicitly call copy() for respective parent.

So, "illegal" means that getParent(), etc. will return null for those copies then, and
getContext() will return the original element's parent?

The places where we had to make modifications in our code were really few.
The question to you is, if you could survive this change in openapi in a
(not next) minor release, or should it be delayed until later?

My personal guess is that not too many depend on the current behavior - and we already did
survive much different API changes ;) If the improvement justifies the risk I'd say go for
it - my 2c.

Out of curiosity: Can you give some numbers and cases where this speed-improvement will be
most noticeable?

Sascha

Sascha,

"Sascha Weinreuter" <sascha.weinreuter@NOSPAM-cit.de> wrote in message
news:dghdct$p8m$1@is.intellij.net...

Eugene Vigdorchik (JetBrains) wrote:

So what I want to do in 5.0.3 (not in 5.0.2) is to copy only the subtree

of

the element. The context of the copy would be the old element's parent,

>

Ok, I would have never expected that the copied element's parent(s) are

also copied. Seems

a bit weird actually

It becomes even more weird when the references in the copy start resolving
to new referent...

>

However it becomes illegal to examine the psi of the copy parents with

e.g. getContainingClass(),

getContainingFile() or getParent(). The code that needs this has to
explicitly call copy() for respective parent.

>

So, "illegal" means that getParent(), etc. will return null for those

copies then, and

getContext() will return the original element's parent?

>

The places where we had to make modifications in our code were really

few.

The question to you is, if you could survive this change in openapi in a
(not next) minor release, or should it be delayed until later?

>

My personal guess is that not too many depend on the current behavior -

and we already did

survive much different API changes ;)

Yeah, this only concerns write aspect of PSI, and e.g. in inspection gadgets
there are no such usages.

If the improvement justifies the risk I'd say go for
it - my 2c.

>

Out of curiosity: Can you give some numbers and cases where this

speed-improvement will be

most noticeable?

Mostly in refactorings and quickfixes. Cannot give the exact numbers, but
those are numerous.

Eugene.

As Sascha said, I had assumed the new behavior of copying the subtree, was the
current behavior. I never tried looking at parents of copied elements because I
assumed it would return null.

Eugene Vigdorchik (JetBrains) wrote:

Hi, Everybody,

We are about to change how PsiElement.copy() works: currently calling this
method copies the tree of the whole file. The reason for this is to have a
constistent tree, so that it would be possible to examine the parent
elements of the copy later. It seems though that most of the clients do not
need to look at parent elements,
and the overhead of copying the whole file is really huge and often
inacceptable.
So what I want to do in 5.0.3 (not in 5.0.2) is to copy only the subtree of
the element. The context of the copy would be the old element's parent, so
resolving references would work as before with the correction that the
references won't resolve to references in the copied file unless the
referenced element is in the copy itself. However it becomes illegal to
examine the psi of the copy parents with e.g. getContainingClass(),
getContainingFile() or getParent(). The code that needs this has to
explicitly call copy() for respective parent.

The places where we had to make modifications in our code were really few.
The question to you is, if you could survive this change in openapi in a
(not next) minor release, or should it be delayed until later?

Eugene.

I'm pretty sure I never used copy() at all, preferring to create java text and then generate tree nodes using the factory. The only exceptions would be PsiWhiteSpace, which can't be created by factory. I should be fine with this, I'm pretty sure.

--Dave Griffith

Eugene Vigdorchik (JetBrains) wrote:

We are about to change how PsiElement.copy() works: currently calling this
method copies the tree of the whole file.

Indeed, I expected copy() to already work like this. I have peformance
problems with two of my intentions, which might be caused in part by
this. Thanks for the info and I welcome the change.

Bas

Really an strange way of copying an tree :) Never guessed this.
I vote for "change it"

Eugene Vigdorchik (JetBrains) schrieb:

Hi, Everybody,

We are about to change how PsiElement.copy() works: currently calling this
method copies the tree of the whole file. The reason for this is to have a
constistent tree, so that it would be possible to examine the parent
elements of the copy later. It seems though that most of the clients do not
need to look at parent elements,
and the overhead of copying the whole file is really huge and often
inacceptable.
So what I want to do in 5.0.3 (not in 5.0.2) is to copy only the subtree of
the element. The context of the copy would be the old element's parent, so
resolving references would work as before with the correction that the
references won't resolve to references in the copied file unless the
referenced element is in the copy itself. However it becomes illegal to
examine the psi of the copy parents with e.g. getContainingClass(),
getContainingFile() or getParent(). The code that needs this has to
explicitly call copy() for respective parent.

The places where we had to make modifications in our code were really few.
The question to you is, if you could survive this change in openapi in a
(not next) minor release, or should it be delayed until later?

Eugene.