Fix CPUFeature iterator behaviour.
The `++` operators should return iterators, not values.
This also updates tests to match, and makes wider use of C++11
range-based `for` loops, where they simplify code.
Change-Id: I2c8ef422e851d6b16c8de2890ae16fc69817a738
diff --git a/src/cpu-features.cc b/src/cpu-features.cc
index ea1e0d3..5c95c20 100644
--- a/src/cpu-features.cc
+++ b/src/cpu-features.cc
@@ -177,12 +177,9 @@
}
CPUFeatures::const_iterator CPUFeatures::begin() const {
- if (features_ == 0) return const_iterator(this, kNone);
-
- int feature_number = CountTrailingZeros(features_);
- vixl::CPUFeatures::Feature feature =
- static_cast<CPUFeatures::Feature>(feature_number);
- return const_iterator(this, feature);
+ // For iterators in general, it's undefined to increment `end()`, but here we
+ // control the implementation and it is safe to do this.
+ return ++end();
}
CPUFeatures::const_iterator CPUFeatures::end() const {
@@ -190,11 +187,11 @@
}
std::ostream& operator<<(std::ostream& os, const CPUFeatures& features) {
- CPUFeatures::const_iterator it = features.begin();
- while (it != features.end()) {
- os << *it;
- ++it;
- if (it != features.end()) os << ", ";
+ bool need_separator = false;
+ for (CPUFeatures::Feature feature : features) {
+ if (need_separator) os << ", ";
+ need_separator = true;
+ os << feature;
}
return os;
}
@@ -205,7 +202,7 @@
return (cpu_features_ == other.cpu_features_) && (feature_ == other.feature_);
}
-CPUFeatures::Feature CPUFeaturesConstIterator::operator++() { // Prefix
+CPUFeaturesConstIterator& CPUFeaturesConstIterator::operator++() { // Prefix
VIXL_ASSERT(IsValid());
do {
// Find the next feature. The order is unspecified.
@@ -219,11 +216,11 @@
// cpu_features_->Has(kNone) is always true, so this will terminate even if
// the features list is empty.
} while (!cpu_features_->Has(feature_));
- return feature_;
+ return *this;
}
-CPUFeatures::Feature CPUFeaturesConstIterator::operator++(int) { // Postfix
- CPUFeatures::Feature result = feature_;
+CPUFeaturesConstIterator CPUFeaturesConstIterator::operator++(int) { // Postfix
+ CPUFeaturesConstIterator result = *this;
++(*this);
return result;
}
diff --git a/src/cpu-features.h b/src/cpu-features.h
index cbec565..738fa69 100644
--- a/src/cpu-features.h
+++ b/src/cpu-features.h
@@ -378,8 +378,8 @@
bool operator!=(const CPUFeaturesConstIterator& other) const {
return !(*this == other);
}
- CPUFeatures::Feature operator++();
- CPUFeatures::Feature operator++(int);
+ CPUFeaturesConstIterator& operator++();
+ CPUFeaturesConstIterator operator++(int);
CPUFeatures::Feature operator*() const {
VIXL_ASSERT(IsValid());
diff --git a/test/test-api.cc b/test/test-api.cc
index b357945..577a8ee 100644
--- a/test/test-api.cc
+++ b/test/test-api.cc
@@ -262,20 +262,20 @@
// Incrementable
// - Incrementing has no effect on an empty CPUFeatures.
- VIXL_CHECK(it3++ == CPUFeatures::kNone);
- VIXL_CHECK(++it3 == CPUFeatures::kNone);
+ VIXL_CHECK(*it3++ == CPUFeatures::kNone);
+ VIXL_CHECK(*(++it3) == CPUFeatures::kNone);
VIXL_CHECK(it3 == It(&f3, CPUFeatures::kNone));
// - Incrementing moves to the next feature, wrapping around (through kNone).
// This test will need to be updated if the Feature enum is reordered.
- VIXL_CHECK(it2_neon++ == CPUFeatures::kNEON);
- VIXL_CHECK(it2_neon++ == CPUFeatures::kCRC32);
- VIXL_CHECK(it2_neon++ == CPUFeatures::kNone);
- VIXL_CHECK(it2_neon++ == CPUFeatures::kFP);
+ VIXL_CHECK(*it2_neon++ == CPUFeatures::kNEON);
+ VIXL_CHECK(*it2_neon++ == CPUFeatures::kCRC32);
+ VIXL_CHECK(*it2_neon++ == CPUFeatures::kNone);
+ VIXL_CHECK(*it2_neon++ == CPUFeatures::kFP);
VIXL_CHECK(it2_neon == It(&f2, CPUFeatures::kNEON));
- VIXL_CHECK(++it2_crc32 == CPUFeatures::kNone);
- VIXL_CHECK(++it2_crc32 == CPUFeatures::kFP);
- VIXL_CHECK(++it2_crc32 == CPUFeatures::kNEON);
- VIXL_CHECK(++it2_crc32 == CPUFeatures::kCRC32);
+ VIXL_CHECK(*(++it2_crc32) == CPUFeatures::kNone);
+ VIXL_CHECK(*(++it2_crc32) == CPUFeatures::kFP);
+ VIXL_CHECK(*(++it2_crc32) == CPUFeatures::kNEON);
+ VIXL_CHECK(*(++it2_crc32) == CPUFeatures::kCRC32);
VIXL_CHECK(it2_crc32 == It(&f2, CPUFeatures::kCRC32));
}
@@ -313,7 +313,6 @@
}
VIXL_CHECK(f3_list.size() == 0);
-#if __cplusplus >= 201103L
std::vector<CPUFeatures::Feature> f2_list_cxx11;
for (auto&& feature : f2) {
f2_list_cxx11.push_back(feature);
@@ -328,16 +327,18 @@
f3_list_cxx11.push_back(feature);
}
VIXL_CHECK(f3_list_cxx11.size() == 0);
-#endif
}
TEST(CPUFeatures_empty) {
// A default-constructed CPUFeatures has no features enabled.
- CPUFeatures f;
- for (CPUFeatures::const_iterator it = f.begin(); it != f.end(); ++it) {
+ CPUFeatures features;
+ for (auto f : features) {
+ USE(f);
VIXL_ABORT();
}
+ VIXL_CHECK(features.HasNoFeatures());
+ VIXL_CHECK(features.Count() == 0);
}
@@ -384,17 +385,16 @@
const std::set<CPUFeatures::Feature>& unexpected,
const std::set<CPUFeatures::Feature>& expected) {
// Print a helpful diagnostic before checking the result.
- typedef std::set<CPUFeatures::Feature>::const_iterator It;
if (!unexpected.empty()) {
std::cout << "Unexpected features:\n";
- for (It it = unexpected.begin(); it != unexpected.end(); ++it) {
- std::cout << " " << *it << "\n";
+ for (auto f : unexpected) {
+ std::cout << " " << f << "\n";
}
}
if (!expected.empty()) {
std::cout << "Missing features:\n";
- for (It it = expected.begin(); it != expected.end(); ++it) {
- std::cout << " " << *it << "\n";
+ for (auto f : expected) {
+ std::cout << " " << f << "\n";
}
}
VIXL_CHECK(unexpected.empty() && expected.empty());
@@ -402,28 +402,29 @@
TEST(CPUFeatures_predefined_legacy) {
- CPUFeatures f = CPUFeatures::AArch64LegacyBaseline();
+ CPUFeatures features = CPUFeatures::AArch64LegacyBaseline();
std::set<CPUFeatures::Feature> unexpected;
std::set<CPUFeatures::Feature> expected;
expected.insert(CPUFeatures::kFP);
expected.insert(CPUFeatures::kNEON);
expected.insert(CPUFeatures::kCRC32);
- for (CPUFeatures::const_iterator it = f.begin(); it != f.end(); ++it) {
- if (expected.erase(*it) == 0) unexpected.insert(*it);
+ for (auto f : features) {
+ if (expected.erase(f) == 0) unexpected.insert(f);
}
CPUFeaturesPredefinedResultCheckHelper(unexpected, expected);
}
TEST(CPUFeatures_predefined_all) {
- CPUFeatures f = CPUFeatures::All();
+ CPUFeatures features = CPUFeatures::All();
std::set<CPUFeatures::Feature> found;
- for (CPUFeatures::const_iterator it = f.begin(); it != f.end(); ++it) {
- found.insert(*it);
+ for (auto f : features) {
+ found.insert(f);
}
VIXL_CHECK(found.size() == CPUFeatures::kNumberOfFeatures);
+ VIXL_CHECK(found.size() == features.Count());
}
// The CPUFeaturesScope constructor is templated, and needs an object which