Fix search result consistency in PositionTracker#3802
Fix search result consistency in PositionTracker#3802iloveeclipse merged 2 commits intoeclipse-platform:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses sporadic search test failures by preventing AbstractTextSearchResult’s internal ConcurrentSkipListSet from becoming inconsistent when PositionTracker mutates Match offsets/lengths, ensuring matches are re-indexed correctly after document edits.
Changes:
- Re-index mutated matches in
PositionTracker.dirtyStateChanged()by removing and re-adding them to the owningAbstractTextSearchResult. - Add a regression test intended to reproduce/guard against match-set corruption after shifting many matches in a file.
- Expand the test search scope to include
*.txtand create a workspace file with many matches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
bundles/org.eclipse.search/newsearch/org/eclipse/search2/internal/ui/text/PositionTracker.java |
Removes/re-adds matches when their offset/length changes so the ordered match set remains consistent. |
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/PositionTrackerTest.java |
Adds a new regression test and workspace setup to generate a file with many matches and validate match removal after reindexing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vogella
force-pushed
the
fix-position-tracker-corruption
branch
from
b895edd to
7f5854f
Compare
|
@iloveeclipse pls review The rest of the copilot feedback was not valuable AFAICT |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@iloveeclipse fine to merge? |
There are few comments from copilot, if they are irrelevant, please resolve them, if they are relevant, please fix. |
|
I think the were irrelevent |
Please go over PR and "resolve conversation" for all opened comments which are irrelevant. |
Done, FYI https://developers.google.com/gemini-code-assist/docs/review-repo-code results IMHO in way better reviews than Copilot, at least 1-2 months ago then I compared both. So in case you experimenting with automatic code reviews, I suggest you give Gemini code review a try. |
I just use whatever is available for me & for github, and as of today it's Copilot only. Beside this, I don't plan to use other IDE's for Eclipse work now, as using Eclipse allows me to test what we produce ("eat your own dog food"). |
|
Just looking at the commit message, can you please update it by following some basic rules:
If you wonder why this matters - it saves reviewer & maintainer time. In this case I had to search which commit was related, which code was originally changed, why the other place was not updated etc. Later we might need to do exact same to fix this PR :-) |
Gemini code review is not another ide it provides feedback on pr |
vogella
force-pushed
the
fix-position-tracker-corruption
branch
from
7f5854f to
fdec281
Compare
|
Commit message updated |
iloveeclipse
left a comment
iloveeclipse
left a comment
There was a problem hiding this comment.
The code change is functionally OK, but we should refactor it a bit & add few comments in different places for later.
When PositionTracker updates match offsets/lengths in dirtyStateChanged(), mutating matches in-place corrupts the ConcurrentSkipListSet ordering introduced in 300659d / Search View: Performance issues on remove and sort eclipse-platform#3733. Fix by removing and re-adding matches whose offset or length changed, so the set re-indexes them correctly. Fixes eclipse-platform#3796
- added `updateMatch(Match match, Position pos)` to manage the `Match` changes - Refactored `PositionTracker` & `AbstractTextSearchResult` - Added javadoc comments where needed See eclipse-platform#3796
iloveeclipse
force-pushed
the
fix-position-tracker-corruption
branch
from
fdec281 to
2f7f85f
Compare
This PR fixes ConcurrentSkipListSet corruption in AbstractTextSearchResult by ensuring matches are correctly re-indexed when mutated by PositionTracker. See #3796