Add new constructor for KDevelop::DetectedProblem class
ClosedPublic

Authored by antonanikin on Mar 2 2017, 4:16 AM.

Details

Summary

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.

Test Plan

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.
antonanikin created this revision.Mar 2 2017, 4:16 AM

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.

mwolff requested changes to this revision.Mar 5 2017, 7:31 PM
mwolff added a subscriber: mwolff.

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

This revision now requires changes to proceed.Mar 5 2017, 7:31 PM
antonanikin updated this revision to Diff 12223.Mar 6 2017, 3:45 AM
antonanikin edited edge metadata.
  • Add Kdevelop::PluginProblem class
antonanikin retitled this revision from Add IProblem::setPluginName() method to Add KDevelop::PluginProblem class.Mar 6 2017, 3:49 AM
antonanikin edited the summary of this revision. (Show Details)
antonanikin edited the test plan for this revision. (Show Details)

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.

kfunk requested changes to this revision.Mar 7 2017, 9:05 PM
kfunk added a subscriber: kfunk.

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 :)

This revision now requires changes to proceed.Mar 7 2017, 9:05 PM
antonanikin updated this revision to Diff 12312.Mar 9 2017, 1:49 AM
antonanikin edited edge metadata.
  • Merge PluginProblem with DetectedProblem
antonanikin edited the summary of this revision. (Show Details)Mar 9 2017, 1:52 AM
antonanikin marked an inline comment as done.
antonanikin retitled this revision from Add KDevelop::PluginProblem class to Add new constructor for KDevelop::DetectedProblem class.

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 :)

kfunk accepted this revision.Mar 12 2017, 10:31 PM

LGTM.

@mwolff: ACK?

shell/problem.cpp
77–78

Side note: We should just do return ... in each case here...

  • Fix inline comments
antonanikin marked an inline comment as done.Mar 13 2017, 4:15 AM
kfunk added a comment.Mar 13 2017, 6:50 AM

Just go for it. Thanks!

This revision was automatically updated to reflect the committed changes.