From c1e3cd01cd4239cf615a9eca5cc52ced616b6d55 Mon Sep 17 00:00:00 2001 From: Herb Sutter Date: Mon, 22 Jan 2018 11:16:11 -0800 Subject: [PATCH] Add gsl::index, closes #1098 (#1115) * Add gsl::index, closes #1098 And update examples throughout to use `index` as appropriate * Actually adding `index` to the GSL.util section * Added `sizeof` to whitelisted signed/unsigned comparisons Same reason as container `.size()` -- better backward compatibility with the existing standard --- CppCoreGuidelines.md | 101 +++++++++++++++++++++++++++---------------- 1 file changed, 64 insertions(+), 37 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 85aa956..26e622f 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -480,14 +480,16 @@ What is expressed in code has defined semantics and can (in principle) be checke The first declaration of `month` is explicit about returning a `Month` and about not modifying the state of the `Date` object. The second version leaves the reader guessing and opens more possibilities for uncaught bugs. -##### Example +##### Example; bad + +This loop is a restricted form of `std::find`: void f(vector& v) { string val; cin >> val; // ... - int index = -1; // bad + int index = -1; // bad, plus should use gsl::index for (int i = 0; i < v.size(); ++i) { if (v[i] == val) { index = i; @@ -497,7 +499,8 @@ The second version leaves the reader guessing and opens more possibilities for u // ... } -That loop is a restricted form of `std::find`. +##### Example; good + A much clearer expression of intent would be: void f(vector& v) @@ -579,7 +582,7 @@ Unless the intent of some code is stated (e.g., in names or comments), it is imp ##### Example - int i = 0; + gsl::index i = 0; while (i < v.size()) { // ... do something with v[i] ... } @@ -1020,7 +1023,7 @@ Time and space that you spend well to achieve a goal (e.g., speed of development X x; x.ch = 'a'; x.s = string(n); // give x.s space for *p - for (int i = 0; i < x.s.size(); ++i) x.s[i] = buf[i]; // copy buf into x.s + for (gsl::index i = 0; i < x.s.size(); ++i) x.s[i] = buf[i]; // copy buf into x.s delete[] buf; return x; } @@ -3260,7 +3263,7 @@ A `span` represents a range of elements, but how do we manipulate elements of th for (int x : s) cout << x << '\n'; // C-style traversal (potentially checked) - for (int i = 0; i < s.size(); ++i) cout << s[i] << '\n'; + for (gsl::index i = 0; i < s.size(); ++i) cout << s[i] << '\n'; // random access (potentially checked) s[7] = 9; @@ -3565,7 +3568,7 @@ The language guarantees that a `T&` refers to an object, so that testing for `nu array w; // ... public: - wheel& get_wheel(size_t i) { Expects(i < w.size()); return w[i]; } + wheel& get_wheel(int i) { Expects(i < w.size()); return w[i]; } // ... }; @@ -9526,7 +9529,7 @@ Arithmetic rules: * [ES.104: Don't underflow](#Res-underflow) * [ES.105: Don't divide by zero](#Res-zero) * [ES.106: Don't try to avoid negative values by using `unsigned`](#Res-nonnegative) -* [ES.107: Don't use `unsigned` for subscripts](#Res-subscripts) +* [ES.107: Don't use `unsigned` for subscripts, prefer `gsl::index`](#Res-subscripts) ### ES.1: Prefer the standard library to other libraries and to "handcrafted code" @@ -9722,7 +9725,7 @@ Conventional short, local names increase readability: template // good void print(ostream& os, const vector& v) { - for (int i = 0; i < v.size(); ++i) + for (gsl::index i = 0; i < v.size(); ++i) os << v[i] << '\n'; } @@ -9731,9 +9734,9 @@ An index is conventionally called `i` and there is no hint about the meaning of template // bad: verbose, hard to read void print(ostream& target_stream, const vector& current_vector) { - for (int current_element_index = 0; - current_element_index < current_vector.size(); - ++current_element_index + for (gsl::index current_element_index = 0; + current_element_index < current_vector.size(); + ++current_element_index ) target_stream << current_vector[current_element_index] << '\n'; } @@ -9908,7 +9911,6 @@ Flag variable and constant declarations with multiple declarators (e.g., `int* p Consider: auto p = v.begin(); // vector::iterator - auto s = v.size(); auto h = t.future(); auto q = make_unique(s); auto f = [](int x){ return x + 10; }; @@ -11975,7 +11977,7 @@ Readability. Error prevention. Efficiency. ##### Example - for (int i = 0; i < v.size(); ++i) // bad + for (gsl::index i = 0; i < v.size(); ++i) // bad cout << v[i] << '\n'; for (auto p = v.begin(); p != v.end(); ++p) // bad @@ -11984,13 +11986,13 @@ Readability. Error prevention. Efficiency. for (auto& x : v) // OK cout << x << '\n'; - for (int i = 1; i < v.size(); ++i) // touches two elements: can't be a range-for + for (gsl::index i = 1; i < v.size(); ++i) // touches two elements: can't be a range-for cout << v[i] + v[i - 1] << '\n'; - for (int i = 0; i < v.size(); ++i) // possible side effect: can't be a range-for + for (gsl::index i = 0; i < v.size(); ++i) // possible side effect: can't be a range-for cout << f(v, &v[i]) << '\n'; - for (int i = 0; i < v.size(); ++i) { // body messes with loop variable: can't be a range-for + for (gsl::index i = 0; i < v.size(); ++i) { // body messes with loop variable: can't be a range-for if (i % 2 == 0) continue; // skip even elements else @@ -12027,7 +12029,7 @@ Readability: the complete logic of the loop is visible "up front". The scope of ##### Example - for (int i = 0; i < vec.size(); i++) { + for (gsl::index i = 0; i < vec.size(); i++) { // do work } @@ -12526,11 +12528,13 @@ It is harder to spot the problem in more realistic examples. ##### Note Unfortunately, C++ uses signed integers for array subscripts and the standard library uses unsigned integers for container subscripts. -This precludes consistency. +This precludes consistency. Use `gsl::index` for subscripts; [see ES.107](#Res-subscripts). ##### Enforcement -Compilers already know and sometimes warn. +* Compilers already know and sometimes warn. +* (To avoid noise) Do not flag on a mixed signed/unsigned comparison where one of the arguments is `sizeof` or a call to container `.size()` and the other is `ptrdiff_t`. + ### ES.101: Use unsigned types for bit manipulation @@ -12597,25 +12601,29 @@ is going to be surprising for many programmers. ##### Example The standard library uses unsigned types for subscripts. -The build-in array uses signed types for subscripts. +The built-in array uses signed types for subscripts. This makes surprises (and bugs) inevitable. int a[10]; for (int i = 0; i < 10; ++i) a[i] = i; vector v(10); - // compares signed to unsigned; some compilers warn - for (int i = 0; v.size() < 10; ++i) v[i] = i; + // compares signed to unsigned; some compilers warn, but we should not + for (gsl::index i = 0; v.size() < 10; ++i) v[i] = i; int a2[-2]; // error: negative size // OK, but the number of ints (4294967294) is so large that we should get an exception vector v2(-2); + Use `gsl::index` for subscripts; [see ES.107](#Res-subscripts). + ##### Enforcement * Flag mixed signed and unsigned arithmetic * Flag results of unsigned arithmetic assigned to or printed as signed. * Flag unsigned literals (e.g. `-2`) used as container subscripts. +* (To avoid noise) Do not flag on a mixed signed/unsigned comparison where one of the arguments is `sizeof` or a call to container `.size()` and the other is `ptrdiff_t`. + ### ES.103: Don't overflow @@ -12779,33 +12787,47 @@ For example Hard: there is a lot of code using `unsigned` and we don't offer a practical positive number type. -### ES.107: Don't use `unsigned` for subscripts +### ES.107: Don't use `unsigned` for subscripts, prefer `gsl::index` ##### Reason To avoid signed/unsigned confusion. To enable better optimization. To enable better error detection. +To avoid the pitfalls with `auto` and `int`. ##### Example, bad - vector vec {1, 2, 3, 4, 5}; + vector vec = /*...*/; - for (int i = 0; i < vec.size(); i += 2) // mix int and unsigned + for (int i = 0; i < vec.size(); i += 2) // may not be big enough cout << vec[i] << '\n'; for (unsigned i = 0; i < vec.size(); i += 2) // risk wraparound cout << vec[i] << '\n'; + for (auto i = 0; i < vec.size(); i += 2) // may not be big enough + cout << vec[i] << '\n'; for (vector::size_type i = 0; i < vec.size(); i += 2) // verbose cout << vec[i] << '\n'; - for (auto i = 0; i < vec.size(); i += 2) // mix int and unsigned + for (auto i = vec.size()-1; i >= 0; i -= 2) // bug + cout << vec[i] << '\n'; + for (int i = vec.size()-1; i >= 0; i -= 2) // may not be big enough + cout << vec[i] << '\n'; + +##### Example, good + + vector vec = /*...*/; + + for (gsl::index i = 0; i < vec.size(); i += 2) // ok + cout << vec[i] << '\n'; + for (gsl::index i = vec.size()-1; i >= 0; i -= 2) // ok cout << vec[i] << '\n'; ##### Note The built-in array uses signed subscripts. The standard-library containers use unsigned subscripts. -Thus, no perfect and fully compatible solution is possible. -Given the known problems with unsigned and signed/unsigned mixtures, better stick to (signed) integers. +Thus, no perfect and fully compatible solution is possible (unless and until the standard-library containers change to use signed subscripts someday in the future). +Given the known problems with unsigned and signed/unsigned mixtures, better stick to (signed) integers of a sufficient size, which is guaranteed by `gsl::index`. ##### Example @@ -12813,7 +12835,7 @@ Given the known problems with unsigned and signed/unsigned mixtures, better stic struct My_container { public: // ... - T& operator[](int i); // not unsigned + T& operator[](gsl::index i); // not unsigned // ... }; @@ -12831,7 +12853,11 @@ Alternatives for users ##### Enforcement -Very tricky as long as the standard-library containers get it wrong. +* Very tricky as long as the standard-library containers get it wrong. +* (To avoid noise) Do not flag on a mixed signed/unsigned comparison where one of the arguments is `sizeof` or a call to container `.size()` and the other is `ptrdiff_t`. + + + # Per: Performance @@ -14808,7 +14834,7 @@ C++ implementations tend to be optimized based on the assumption that exceptions int find_index(vector& vec, const string& x) { try { - for (int i = 0; i < vec.size(); ++i) + for (gsl::index i = 0; i < vec.size(); ++i) if (vec[i] == x) throw i; // found x } catch (int i) { return i; @@ -19996,12 +20022,13 @@ for example, `Expects(p != nullptr)` will become `[[expects: p != nullptr]]`. ## GSL.util: Utilities -* `finally` // `finally(f)` makes a `final_action{f}` with a destructor that invokes `f` -* `narrow_cast` // `narrow_cast(x)` is `static_cast(x)` -* `narrow` // `narrow(x)` is `static_cast(x)` if `static_cast(x) == x` or it throws `narrowing_error` -* `[[implicit]]` // "Marker" to put on single-argument constructors to explicitly make them non-explicit. -* `move_owner` // `p = move_owner(q)` means `p = q` but ??? +* `finally` // `finally(f)` makes a `final_action{f}` with a destructor that invokes `f` +* `narrow_cast` // `narrow_cast(x)` is `static_cast(x)` +* `narrow` // `narrow(x)` is `static_cast(x)` if `static_cast(x) == x` or it throws `narrowing_error` +* `[[implicit]]` // "Marker" to put on single-argument constructors to explicitly make them non-explicit. +* `move_owner` // `p = move_owner(q)` means `p = q` but ??? * `joining_thread` // a RAII style version of `std::thread` that joins. +* `index` // a type to use for all container and array indexing (currently an alias for `ptrdiff_t`) ## GSL.concept: Concepts