Add null check for Disassembly view source #295
Merged
Conversation
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
jonahgraham
requested changes
Feb 22, 2023
| // Log error to indicate what is tha cause of the issue | ||
| String warningMessage = "Line(s) " + Integer.toString(first) + "-" + Integer.toString(last) //$NON-NLS-1$ | ||
| + " cannot be found in file: " + fFileKey + ".\nSkipping lines!"; //$NON-NLS-1$ | ||
| DsfUIPlugin.logErrorMessage(warningMessage); //$NON-NLS-1$ |
There was a problem hiding this comment.
This logErrorMessage has two minor problems:
- It discards the exception. I think you should log the exception too, it provides the stack trace/context to diagnose the problem
- What is logged in this case is
Internal message logged from Debug UIas typeINTERAL_ERROR, which seems a little to harsh here.
Suggested change
| DsfUIPlugin.logErrorMessage(warningMessage); //$NON-NLS-1$ | |
| DsfUIPlugin.log(Status.warning(warningMessage, e)); |
There was a problem hiding this comment.
I updated this and the Bundle-Version. Thank you!
|
GitHub doesn't allow me to add a comment to a non-modified file. The build failed with this error: which means this line needs to be changed to |
Disassembly view is expected to be populated with lines that are fetched from a given source file. There might be the case where instructions on what lines to append are wrong. This results in a null response that will propagate through the code leading to a NPE. The current commit is proofing the code from NPE by: - removing the source position of the lines that were not found within the given file - null checking the source before becoming a key element in the code flow - adding logging if expected lines are not found in the given file Resolves: eclipse-cdt#287
jonahgraham
approved these changes
Feb 23, 2023
|
Thanks @LizzMre - this change looks good now. I am waiting on CI to confirm no issues and then it can be merged. |
|
@jonahgraham Thank you very much for your help! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Disassembly view is expected to be populated with lines that are fetched from a given source file. There might be the case where instructions on what lines to append are wrong. This results in a null response that will propagate through the code leading to a NPE.
The current commit is proofing the code from NPE by:
Resolves: #287