The patch adds new constructor for KDevelop::DetectedProblem which allows to avoid creating of subclasses with only overloaded source() and sourceString() methods for "problem-checker" plugins like cppcheck/valgrind.
Details
Tested with master branch and cppcheck/valgrind plugins
Diff Detail
- Repository
- R33 KDevPlatform
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Another way - we can add class PluginProblem which will take plugin name (sourceString() result) at construction and overload source() and sourceString() methods. Then we can drop new method from IProblem and it's subclasses.
As-is, this is not a good approach imo. Either have it as generic API in iproblem.h (which would mean you'd also need to serialize the data in problem.cpp), or have it as a special case for the detected problem and only use it there
interfaces/iproblem.h | ||
---|---|---|
96 ↗ | (On Diff #12046) | I don't understand this part, can you elaborate please? also, I'd say this should be setSourceString then and the default implementation in problem.h should do something as-is, this API is highly confusing |
language/duchain/problem.cpp | ||
216 ↗ | (On Diff #12046) | this should check if the source has been set explicitly, and if so, return that directly |
238 ↗ | (On Diff #12046) | this should do something |
language/duchain/problem.h | ||
149 ↗ | (On Diff #12046) | add APIDOX saying that the default implementation does nothing |
As-is, this is not a good approach imo. Either have it as generic API in iproblem.h (which would mean you'd also need to serialize the data in problem.cpp), or have it as a special case for the detected problem and only use it there
@mwolff, yes, I agree with your remarks about ugly API in previous version. I suggests new version with PluginProblem class which simplifies whole revision.
Sorry Anton, more nitpicks/questions :)
shell/problem.cpp | ||
---|---|---|
207 | Issue a runtime warning maybe? => qCWarning(...) | |
shell/problem.h | ||
133 | I'm wondering if we should just merge PluginProblem with DetectedProblem... Just add DetectedProblem::DetectedProblem(const QString& pluginName) and set source = Plugin in that case). DetectedProblem::sourceString then returns the custom plugin name if source == Plugin. PluginProblem does not really add much to DetectedProblem this looks like unnecessary code bloat. Other opinions? | |
shell/tests/test_pluginproblem.cpp | ||
40 ↗ | (On Diff #12223) | init({{}}) (no joke) -- so we don't load any plugins :) |
I'm wondering if we should just merge PluginProblem with DetectedProblem...
Just add DetectedProblem::DetectedProblem(const QString& pluginName)
Ok, you are right - it's more simple/clear way, thanks :)