Skip to content

Fix #1999 : REFS aren't indexed if DEFS is empty #2624

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.

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

Merged
merged 1 commit into from
Jan 31, 2019

Conversation

idodeclare
Copy link
Contributor

Hello,

Please consider for integration this patch to index REFS independently from DEFS and to bump the affected analyzers' specialized version numbers.

Thank you.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3862

  • 34 of 34 (100.0%) changed or added relevant lines in 31 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.02%) to 73.136%

Files with Coverage Reduction New Missed Lines %
opengrok-indexer/src/main/java/org/opengrok/indexer/history/FileHistoryCache.java 1 73.49%
opengrok-indexer/src/main/java/org/opengrok/indexer/analysis/AnalyzerGuru.java 2 82.99%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/BazaarRepository.java 2 70.16%
Totals Coverage Status
Change from base Build 3858: 0.02%
Covered Lines: 32787
Relevant Lines: 44830

💛 - Coveralls

protected int getSpecializedVersionNo() {
return 20180208_00; // Edit comment above too!
}

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

* work around #1376: symbols search works like full text search.
*/
OGKTextField ref = new OGKTextField(QueryBuilder.REFS,
this.symbolTokenizer);
Copy link
Contributor

Choose a reason for hiding this comment

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

the new line break is not necessary here I think but that's not important

@tulinkry
Copy link
Contributor

Looks ok.

@idodeclare
Copy link
Contributor Author

Thank you for the review, @tulinkry

@tarzanek
Copy link
Contributor

this looks OK, however I don't know if REFS have any value without DEFS in the file (yes, you should get existing symbols in index at least, if local file has none detected)

@tarzanek
Copy link
Contributor

so the question is whether this won't waste more cpu cycles than add value

@tarzanek
Copy link
Contributor

that said I am OK with merge

@tarzanek tarzanek self-assigned this Jan 30, 2019
@tarzanek tarzanek added this to the 1.2 milestone Jan 30, 2019
@vladak vladak merged commit 80c1d96 into oracle:master Jan 31, 2019
@vladak
Copy link
Member

vladak commented Jan 31, 2019

Merged based on Slack #dev discussion.

@idodeclare
Copy link
Contributor Author

this looks OK, however I don't know if REFS have any value without DEFS in the file (yes, you should get existing symbols in index at least, if local file has none detected)

@tarzanek, OpenGrok REFS are of course also useful for linking to definitions in other files.

While in e.g. Java or C it's difficult to come up with a useful, example source code file that has no DEFS from ctags, in Shell for example (and per the linked issue #1999), you can have a source code file that does useful things but which has no aliases, functions, or heredocs itself — and so has no results from ctags — but which includes another file using . and calls the included file's scripts.

Without this change, the first example file's REFS would not be indexed, so the file would not be seen as a caller of the included file's functions.

@idodeclare idodeclare deleted the bugfix/no_defs_no_refs branch February 1, 2019 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants