-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add method to deal with matrix and fix comment #34
Merged
Merged
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
faeffa7
Add method to deal with matrix and fix comment
LoiNguyenCS 430b6f7
modify checkInString
LoiNguyenCS cd27329
modify javadoc
LoiNguyenCS 0ae0ead
add SuppressWarnings
LoiNguyenCS 7eb201f
update to most recent CF version + remove SW + fix formatting + a few…
kelloggm 3b98db6
documentqtion tweak
kelloggm e6caafa
add test for example in Javadoc, and also fix test naming to normal J…
kelloggm 546d2e3
fix formatting violation
kelloggm 7cfa6c0
more standard formatting
kelloggm c56837c
simpler substring
kelloggm 3be417b
more complete tests
kelloggm 66b8c41
fix formatting
kelloggm File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While adding this check here isn't a bad thing to do, the specification of
checkInString
should still be that theindex
parameter ought to be@IndexFor("#2")
: the goal of the checker is to prevent crashes, and the change you've made here just turns anIndexOutOfBoundsException
into aRuntimeException
.We should retain the
@IndexFor
annotation on line 275 above: we still need to deal with the possibility that the index used at some call sites might not be valid (ie, the checker warnings).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. So then I should stick to first approach, which is to suppress the warning and provide a link to the bug report. Is that correct, Professor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Professor, I tried to build a simple test case for the bug report. But then the test case appeared to work, here is the test case:
So I guess maybe there're some problems with my codes that I'm not aware of, I will try to investigate more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: Professor, I have found a test case where we would receive the warning. Here is the test case.
and the warning is
I have made a bug report, here is the link: typetools/checker-framework#5471
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I've fixed the bug, so when we make the next Checker Framework release we'll be able to remove these warning suppressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you Professor. In that case, should I close the issue that I have made?