[eclipse/xtext#1679] ported xtend code to java #1442
Conversation
cdietrich
force-pushed
the
cd_idex2j
branch
6 times, most recently
from
b1b15c5 to
3b85dbb
Compare
| super("testlang"); | ||
| } | ||
|
|
||
| } |
| disposableRegistry.register(this); | ||
| } | ||
|
|
||
| private final Map<String, ExecutorService> instanceCache = Maps |
There was a problem hiding this comment.
You could just do - private final Map<String, ExecutorService> instanceCache = new HashMap<>(3)
| for (ExecutorService executorService : instanceCache.values()) { | ||
| executorService.shutdown(); | ||
| } | ||
| this.instanceCache.clear(); |
| public String toString() { | ||
| ToStringBuilder b = new ToStringBuilder(this); | ||
| b.add("imageIDs", imageIDs); | ||
| return b.toString(); |
There was a problem hiding this comment.
How about replacing the above code with something like return "imageID's :" + imageIDs.toString(), so that we can remove the reference to the ToStringBuilder type?
There was a problem hiding this comment.
i wanted to keep the old impl for all of the toStrings
|
|
||
| @Override | ||
| public String toString() { | ||
| ToStringBuilder b = new ToStringBuilder(this); |
There was a problem hiding this comment.
Can we remove the reference to the ToStringBuilder if possible ?
There was a problem hiding this comment.
can be refactored lated. i want to keep the current behaviour
|
|
||
| public RenameContext(List<? extends RenameChange> changes, ResourceSet resourceSet, | ||
| IChangeSerializer changeSerializer, RefactoringIssueAcceptor issues) { | ||
| super(); |
| private final boolean isFile; | ||
|
|
||
| public ResourceRelocationChange(final URI fromURI, final URI toURI, final boolean isFile) { | ||
| super(); |
|
|
||
| @Override | ||
| public String toString() { | ||
| ToStringBuilder b = new ToStringBuilder(this); |
There was a problem hiding this comment.
Remove reference to ToStringBuilder if possible.
There was a problem hiding this comment.
wanted to keep it as i want to keep the behaviour
|
|
||
| def void afterBuild(List<IResourceDescription.Delta> deltas) | ||
| void afterBuild(List<IResourceDescription.Delta> deltas); | ||
|
|
||
| } |
| if (crossLinkedEObject.eIsProxy()) { | ||
| return null; | ||
| } | ||
| IParseResult parseResult = resource.getParseResult(); |
There was a problem hiding this comment.
I believe, we don't need this check here. The call to resolveCrossReferencedElementAt(...) on line 59 would return a null value if the XtextResource#getParseResult() is null.
| disposableRegistry.register(this); | ||
| } | ||
|
|
||
| private final Map<String, ExecutorService> instanceCache = Maps.newHashMapWithExpectedSize(3); |
There was a problem hiding this comment.
Should be a ConcurrentHashMap, otherwise the synchronization attempt in get(String) is completely flawed
There was a problem hiding this comment.
why? there is a second read in the synchronized
There was a problem hiding this comment.
Thread A calls get, sees nothing and enters the synchronize block, calls get again, sees still nothing and starts to put things into the map (this is not an atomic operation). While the map is being modified, Thread B calls get and sees an inconsistent map state. This can be avoided by using a ConcurrentHashMap + computeIfAbsent
There was a problem hiding this comment.
sry. can we do this in a separate PR. i am not able to think this properly through right now
| } | ||
|
|
||
| public ExecutorService get(String key) { | ||
| ExecutorService result = instanceCache.get(key); |
There was a problem hiding this comment.
instanceCache.computeIfAbsent(..)
| private final List<String> imageIDs; | ||
|
|
||
| public AlternativeImageDescription(Iterable<String> imageIDs) { | ||
| this.imageIDs = ImmutableList.<String>copyOf(imageIDs); |
There was a problem hiding this comment.
Unnecessary type argument
org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/refactoring/IRenameStrategy2.java
Show resolved
Hide resolved
org.eclipse.xtext.ide/src/org/eclipse/xtext/ide/refactoring/IRenameStrategy2.java
Show resolved
Hide resolved
| } | ||
|
|
||
| public ResponseError toResponseError() { | ||
| final RefactoringIssueAcceptor.Severity maxSeverity = getMaximumSeverity(); |
| /** | ||
| * @since 2.22 | ||
| */ | ||
| protected int getCodeBySeverity(final RefactoringIssueAcceptor.Severity maxSeverity) { |
| */ | ||
| public class ServerRefactoringIssueAcceptor implements RefactoringIssueAcceptor { | ||
| public static class Issue { | ||
| private RefactoringIssueAcceptor.Severity severity; |
There was a problem hiding this comment.
Unnecessary qualification I guess.
| public RefactoringIssueAcceptor.Severity getMaximumSeverity() { | ||
| if (issues.size() > 0) { | ||
| ServerRefactoringIssueAcceptor.Issue minBySeverity = IterableExtensions.minBy(issues, (i) -> i.severity); | ||
| RefactoringIssueAcceptor.Severity severity = null; |
There was a problem hiding this comment.
since the issues iterable is not empty, minBySeverity cannot be null.
Could be simplified to return IterableExtensions.minBy(..).severity
|
|
||
| public RefactoringIssueAcceptor.Severity getMaximumSeverity() { | ||
| if (issues.size() > 0) { | ||
| ServerRefactoringIssueAcceptor.Issue minBySeverity = IterableExtensions.minBy(issues, (i) -> i.severity); |
There was a problem hiding this comment.
Maybe use Ordering.min instead.
cdietrich
force-pushed
the
cd_idex2j
branch
3 times, most recently
from
3be63c5 to
1f054c0
Compare
Signed-off-by: Christian Dietrich <[email protected]>
[eclipse-xtext/xtext#1679] ported xtend code to java
Signed-off-by: Christian Dietrich [email protected]