Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code cleanup #1006

Merged
merged 9 commits into from
May 21, 2024
Merged

Code cleanup #1006

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
17d26eb
refact: Improve Collection#toArray() calls
sebthom May 21, 2024
f78ad81
refact: Replace unnecessary use of Optional.ofNullable
sebthom May 21, 2024
bec6239
refact: Fix typos
sebthom May 21, 2024
b2bd321
refact: Replace Consumer<Long> with LongConsumer
sebthom May 21, 2024
0afd359
refact: avoid repeated URI parsing when constructing URIMatch
sebthom May 21, 2024
d7eba1e
refact: remove redundant semicolon
sebthom May 21, 2024
4c16f90
refact: use diamond operator
sebthom May 21, 2024
84a7411
refact: make org.eclipse.lsp4e.internal.Pair a record class
sebthom May 21, 2024
3011d21
refact: remove not-thrown exceptions from `throws` clauses
sebthom May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/references/LSSearchQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*******************************************************************************/
package org.eclipse.lsp4e.operations.references;

import java.net.URISyntaxException;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.CompletableFuture;
Expand Down Expand Up @@ -148,11 +149,11 @@ private static Match toMatch(@NonNull Location location) {
return new FileMatch((IFile) resource, 0, 0, lineEntry);
}
try {
return new URIMatch(location);
} catch (BadLocationException ex) {
return URIMatch.create(location);
} catch (BadLocationException | URISyntaxException ex) {
LanguageServerPlugin.logError(ex);
return null;
}
}
}

@Override
Expand Down
20 changes: 13 additions & 7 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/references/URIMatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,28 @@
package org.eclipse.lsp4e.operations.references;

import java.net.URI;
import java.net.URISyntaxException;

import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jface.text.BadLocationException;
import org.eclipse.jface.text.IDocument;
import org.eclipse.lsp4e.LSPEclipseUtils;
import org.eclipse.lsp4j.Location;
import org.eclipse.search.ui.text.Match;

public class URIMatch extends Match {

public final @NonNull Location location;
public static URIMatch create(final Location location) throws BadLocationException, URISyntaxException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with sticking to a constructor here?

Copy link
Contributor Author

@sebthom sebthom May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the first invocation in a constructor needs to be a super() call, that's why the current implementation parses the URI three times instead of once and then reusing the uri object to construct arguments for the parent constructor. which is very inefficient.

That is what we have currently:

public URIMatch(@NonNull Location location) throws BadLocationException {
	super(
		URI.create(location.getUri()),
		LSPEclipseUtils.toOffset(location.getRange().getStart(), LSPEclipseUtils.getDocument(URI.create(location.getUri()))),
		LSPEclipseUtils.toOffset(location.getRange().getEnd(),  LSPEclipseUtils.getDocument(URI.create(location.getUri()))) - LSPEclipseUtils.toOffset(location.getRange().getStart(), LSPEclipseUtils.getDocument(URI.create(location.getUri()))));
	this.location = location;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks. Can't wait for https://openjdk.org/jeps/447 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks. Can't wait for openjdk.org/jeps/447 ;)

Me too!

final URI uri = new URI(location.getUri());
final IDocument doc = LSPEclipseUtils.getDocument(uri);
final int offset = LSPEclipseUtils.toOffset(location.getRange().getStart(), doc);
final int length = LSPEclipseUtils.toOffset(location.getRange().getEnd(), doc) - LSPEclipseUtils.toOffset(location.getRange().getStart(), doc);
return new URIMatch(location, uri, offset, length);
}

public final Location location;

public URIMatch(@NonNull Location location) throws BadLocationException {
super(URI.create(location.getUri()), //
LSPEclipseUtils.toOffset(location.getRange().getStart(), LSPEclipseUtils.getDocument(URI.create(location.getUri()))),
LSPEclipseUtils.toOffset(location.getRange().getEnd(), LSPEclipseUtils.getDocument(URI.create(location.getUri()))) - LSPEclipseUtils.toOffset(location.getRange().getStart(), LSPEclipseUtils.getDocument(URI.create(location.getUri()))));
protected URIMatch(final Location location, final URI uri, final int offset, final int length) {
super(uri, offset, length);
this.location = location;
}

}