Add new CodeChecker resolution logic #170
Conversation
| } | ||
|
|
||
| monitor.beginTask("Starting Analysis...", taskCount.get() * 2); | ||
| config.getCodeChecker().analyze(logFile, true, monitor, taskCount.get() * 2, config); |
There was a problem hiding this comment.
TODO There is a npe thrown at this line, when there is no CodeChecker configured.
gamesh411
left a comment
gamesh411
left a comment
There was a problem hiding this comment.
Nice patch! Please see my remarks inline.
....codechecker.eclipse.plugin/src/org/codechecker/eclipse/plugin/codechecker/ICodeChecker.java
Show resolved
Hide resolved
| * | ||
| * @return The checker list. | ||
| */ | ||
| public String getCheckers(); |
There was a problem hiding this comment.
I would say, that getCheckers, getVersion should return an Optional, an analyze could indicate with an exception if something went wrong. Feel free to apply these suggestions liberally, at your discretion.
There was a problem hiding this comment.
I'm not completely convinced about the Optional usage in this case, as the version information should not be absent, if there is a valid CodeChecker package at that location. Same situation with the checkers. Maybe I could annotate these methods with @nonnull to be more clear.
Related: I'm still thinking about extracting the package validation section from the getVersion method. I'm just not sure whether to do it on the interface level, or in the implementation. (This getVersion currently not used in the plugin, but could be useful in the future for example to check compatibility).
....codechecker.eclipse.plugin/src/org/codechecker/eclipse/plugin/codechecker/ICodeChecker.java
Outdated
Show resolved
Hide resolved
...org/codechecker/eclipse/plugin/codechecker/locator/CustomBuiltCodeCheckerLocatorService.java
Outdated
Show resolved
Hide resolved
...gin/src/org/codechecker/eclipse/plugin/codechecker/locator/EnvCodeCheckerLocatorService.java
Outdated
Show resolved
Hide resolved
...rc/org/codechecker/eclipse/plugin/codechecker/locator/PreBuiltCodeCheckerLocatorService.java
Outdated
Show resolved
Hide resolved
| * The query string. | ||
| * @return The matching ResolutionMethodTypes if exists null otherwise. | ||
| */ | ||
| public static ResolutionMethodTypes getFromString(String s) { |
There was a problem hiding this comment.
If the enum symbol name is the same as the String it represents (for example in this case), then getFromString, and even the getDefault value is redundant. Consider using an enum without a base type, and use the toString() and valueOf(String) methods. (To further clarify: this would effectively reduce the enum definition to a 3 member plain enum).
There was a problem hiding this comment.
Darn, There is an another enum which could be simplified like this. Thanks for the remark, I'll make a patch for that other.
...org.codechecker.eclipse.plugin/src/org/codechecker/eclipse/plugin/report/job/AnalyzeJob.java
Show resolved
Hide resolved
|
Thanks @gamesh411 for your thorough review! I'll update the found issues. |
|
#180 Should be merged before this. |
Refactored the logic, how CodeChecker is found and configured. There is now an internal representation of a CodeChecker. There are a locator service introduced to handle CodeChecker resolution. There are a new gui radio button group to select how CodeChecker should be found. There is a proper dynamic widget enablement function depending on the selected options, and more verbose messages for the preferences panel.
New Unit tests and extended Integration tests. The CodeChecker class, and the locator logic is tested for PATH and Pre built package resolution. This is the same for the Integration tests, so only these two are tested. The Custom built CodeChecker package with virtual env needs a virtual environment emulation, which is not implemented yet.
3 participants
Extended CodeChecker configuration capabilities of the Plugin.
There is a group of three radio button in the beginning of the Preferences/Project properties page.
The messages of this page now are meaningful eg: Displays the currently used CodeChecker or emit a helpful error message.
There are some changes under the hood.
The CodeChecker class.
The CodeCheckerLocator class group.
This resolves #152, also resolves #162