| Summary: | src/dbus/server/server.cpp:495: possible bad if test ? | ||
|---|---|---|---|
| Product: | SyncEvolution | Reporter: | dcb314 |
| Component: | SyncEvolution | Assignee: | Patrick Ohly <patrick.ohly> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | CC: | syncevolution-issues |
| Version: | 1.3.99.3 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
Which code analyzer found that? I run Klocwork occasionally and analyzed all problems in SyncEvolution code itself that Klocwork reported. In libsynthesis the code triggered more warnings. The false negative rate was higher than in SyncEvolution due to the different coding style, so I only checked some issues there.
Anyway, I don't remember this one.
The code is the result of an invalid rewrite:
- if (it->lock()->getPriority() <= session->getPriority()) {
+ // skip over dead sessions, they will get cleaned up elsewhere
+ boost::shared_ptr<Session> session = it->lock();
+ if (session && session->getPriority() <= session->getPriority()) {
The session variable here shadows the session parameter, which clearly isn't right. It's not that important, though, because session priorities aren't used much (not at all?).
I'll fix it. Thanks for reporting this.
>Which code analyzer found that? Sorry, I should have mentioned that. cppcheck, as in http://cppcheck.sourceforge.net/ I find it a useful tool. (In reply to comment #2) > >Which code analyzer found that? > > Sorry, I should have mentioned that. > > cppcheck, as in http://cppcheck.sourceforge.net/ > > I find it a useful tool. Agreed. I started applying it to libsynthesis, activesyncd and of course SyncEvolution as part of the nightly testing. After some less important changes (performance, unnecessary writes, style), a handful of suppressions and two real fixes, the code passes. I'll notice regressions, too. The other real problem was: Author: Patrick Ohly <patrick.ohly@intel.com> 2014-01-07 11:04:22 Committer: Patrick Ohly <patrick.ohly@intel.com> 2014-01-17 15:35:55 Parent: 41f917d912c0a0a066e61ac07bd23492ca58af01 (testing: cppcheck redundant assignment) Child: a20a26df4b4ea3870d92982651ff0899bb595034 (cppcheck: suppress warnings) Branch: for-master/testing Follows: syncevolution-1-3-99-6 Precedes: testing: fix naming of log files in doCopy() The log file guard instance which should have added a "copy" part to log files was deleted again before it could have the desired effect. Found by cppcheck ("Instance of 'SyncPrefix' object is destroyed immediately."). ----------------------------- test/ClientTest.cpp ----------------------------- index ab6a12f..e9d874d 100644 @@ -2977,9 +2977,9 @@ void SyncTests::deleteAll(DeleteAllMode mode) { } /** get both clients in sync with empty server, then copy one item from client A to B */ void SyncTests::doCopy() { - SyncPrefix("copy", *this); + SyncPrefix copy("copy", *this); // check requirements CT_ASSERT(accessClientB); Fixes and cppcheck-enhanced test scripts were merged into master. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
I just ran the static analyser over syncevolution-1.3.99.3 It said many things, including [src/dbus/server/server.cpp:495] -> [src/dbus/server/server.cpp:495]: (style) Same expression on both sides of '<='. Source code is if (session && session->getPriority() <= session->getPriority()) { Suggest code rework. I notice there is a parameter of the routine called session and a local variable with the same name. Odd. I also checked the newest source code and the bug appears there too.