Page MenuHomePhabricator

unsigned count() for ADT
ClosedPublic

Authored by yaron.keren on Jun 4 2014, 7:39 AM.

Details

Reviewers
dblaikie
rsmith
Summary

The count() function for STL datatypes returns unsigned, even where it's only 1/0 result like std::set. Some of the LLVM ADT already return unsigned count(), while others still return bool count().

In continuation to r197879, this patch modifies DenseMap, DenseSet, ScopedHashTable, ValueMap:: count() to return unsigned instead of bool, 1 instead of true and 0 instead of false.

Updated upatch to ToT, fixed comments and two warnings.

Diff Detail

Event Timeline

yaron.keren updated this revision to Diff 10093.Jun 4 2014, 7:39 AM
yaron.keren retitled this revision from to unsigned count() for ADT.
yaron.keren updated this object.
yaron.keren edited the test plan for this revision. (Show Details)
yaron.keren updated this object.Jun 4 2014, 7:41 AM
yaron.keren added a subscriber: Unknown Object (MLST).

From the list discussion, Richard Smith:

If we want our container-like types to be containers, count should return size_type. I suspect that size_type == size_t is probably the best choice (for performance and code size), but I really doubt this makes much difference. I don't think there are many cases where we'd want to support containers of more than 4Gi elements. (In short, I don't think this really matters, but each container should be internally consistent.)

Updated count() and size() type to class-local size_type.

Typo fix in SmallPtrSet.

dblaikie added inline comments.
lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp
311

It's common to write a "does this container contain this thing" as simply boolean testing the result of 'count':

assert(FnDebugInfo.count(GV));

Personally this is easier for me to read because I don't have to worry about "is this container a multimap and are they testing that there's /exactly/ one element" (and writing it as "> 0" when it's a single map is also a little confusing, even though it's semantically equivalent to the non-zero count test above)

Not a requirement, just a though - maybe others feel more strongly.

lib/IR/Metadata.cpp
666

same here, though I guess this is a bit more interesting as in this case the bool on the LHS would be converted to an int and compared... but you're == 1, so it is the same whether it's a multimap or not (ie: it's only true if there's exactly one thing)... not sure how I'd prefer to write this one.

Fixed first test per comment. The second test is trickier, comparing a bool to count() results in VC complaining:

..\..\..\lib\IR\Metadata.cpp(667): warning C4805: '==' : unsafe mix of type 'bool' and type

I modified it to read count() > 0, so the comparison will be between bools.

Is this good to commit?

dblaikie accepted this revision.Jun 19 2014, 7:11 AM
dblaikie added a reviewer: dblaikie.

Looks fine to me. I'd probably have written that last one as "!= 0" rather than "> 0", but either seems acceptable.

This revision is now accepted and ready to land.Jun 19 2014, 7:11 AM

I modified size_type for BitVector back from size_t to unsigned.
The code caused a compile building failure on LP64 systems where sizeof(size_t)==sizeof(long) with template deduction in line 560. The failure could be fixed otherwise but anyhow I don't think that we have the need to store that many bits.

Rereading BitVector.h, if size were unsigned long instead of unsigned then members Size, Capacity should also be unsigned long.

Richard, I reverted size_type to unsigned since size are counts are usually assumed in LLVM code to be unsigned.
We don't really need to 64 bit-sized containers (yet?) so better be safe rather than introduce possibly subtle bugs.