922727 - History.cpp:1558:50: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Daniel Holbert [:dholbert]]

Build warnings for visits.Length() vs. "visitCount" comparisons, in History.cpp:
{
toolkit/components/places/History.cpp:1558:50: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

   bool removingPage = visits.Length() == aEntry->visitCount &&
                                                  ^
toolkit/components/places/History.cpp:1636:34: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

   if (visits.Length() == aEntry->visitCount && !aEntry->bookmarked) {
                                  ^
}

visits.Length() is unsigned, whereas visitCount is signed (and initialized to -1), so comparing them is potentially bogus.

Maybe visitCount should be unsigned (and initialized to UINT32_MAX, if we have to initialize it to a bogus sentinel value)?

Looks like these comparisons were added in this cset for bug 825849:
 http://hg.mozilla.org/mozilla-central/rev/9b891394bb82

Comment 1

[Marco Bonardo [:mak]]

I think the original scope was to assert that they (both visitCount and bookmarked) were always initialized before being read, though those asserts don't exist as of now.
I think the best thing would be to make visitCount a uint (init to 0), bookmarked a bool (init to false) and add an initialized field that we can assert in debug mode.
Well, probably the cleanest thing would be to make both properties private, expose a setter that sets both and initialized, and 2 getters, one for each, that will assert if not initialized.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #1)
> I think the best thing would be to make visitCount a uint (init to 0),
> bookmarked a bool (init to false) and add an initialized field that we can
> assert in debug mode.
> Well, probably the cleanest thing would be to make both properties private,
> expose a setter that sets both and initialized, and 2 getters, one for each,
> that will assert if not initialized.

No one's stepped up to do this yet, so for now, I'd like to punt this suggestion to a code-refactoring followup, and in the meantime, just use static_cast<> to suppress these build warnings (i.e. explicitly making the type-conversions that are already happening under the hood, to avoid the warning-spam).

This is sort of cheating, but it'll let us mark this directory as FAIL_ON_WARNINGS & prevent introduction of more warnings, which IMHO is for the Greater Good.  And when someone gets around to doing the refactoring mentioned in comment 1, all the better. :)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: patch v1: static_cast<size_t>

This static_cast's visitCount to size_t (which is the type that nsTArray::Length() returns nowadays:
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h?rev=7c657a05e066&mark=324-324,328-328#324
)

Comment 4

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ead6c90bde

Comment 5

[Arnaud Sourioux [:Six]]

Hi,

I'm interested to develop it as Mak proposed it in comment 1
I guess i will need to fill a new bug linked to this one

Which solution is the best between case #1 and case #2 in this pastebin?
http://pastebin.com/SGg38WEU
Maybe there is already a templated class in mozilla such as IsSetProperty that i used in Case #2

Thanks,

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Sourioux [:Six] from comment #5)
> I'm interested to develop it as Mak proposed it in comment 1
> I guess i will need to fill a new bug linked to this one

No need for a new bug -- I already spun off bug 1049812 for that purpose.

This bug here should've been closed when its cset was merged to central, but it seems to have been skipped.

This was merged as: https://hg.mozilla.org/mozilla-central/rev/d6ead6c90bde