Bug 72576 - src/dbus/server/server.cpp:495: possible bad if test ?
Summary: src/dbus/server/server.cpp:495: possible bad if test ?
Alias: None
Product: SyncEvolution
Classification: Unclassified
Component: SyncEvolution (show other bugs)
Hardware: All All
: medium normal
Assignee: Patrick Ohly
QA Contact:
Depends on:
Reported: 2013-12-10 19:28 UTC by dcb314
Modified: 2014-01-20 08:37 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Description dcb314 2013-12-10 19:28:58 UTC
I just ran the static analyser over syncevolution-

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.
Comment 1 Patrick Ohly 2013-12-10 20:15:47 UTC
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.
Comment 2 dcb314 2013-12-10 20:31:04 UTC
>Which code analyzer found that?

Sorry, I should have mentioned that.

cppcheck, as in http://cppcheck.sourceforge.net/

I find it a useful tool.
Comment 3 Patrick Ohly 2014-01-17 14:47:03 UTC
(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

    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
Comment 4 Patrick Ohly 2014-01-20 08:37:40 UTC
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.