Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add gsl::index, closes #1098 #1115

Merged
merged 4 commits into from Jan 22, 2018
Merged

Add gsl::index, closes #1098 #1115

merged 4 commits into from Jan 22, 2018

Conversation

hsutter
Copy link
Contributor

@hsutter hsutter commented Dec 26, 2017

(Thanks to many WG21 experts for their comments and feedback into this note.)

Add the following typedef to GSL

namespace gsl { using index = ptrdiff_t; }

and recommend gsl::index for all container indexes/subscripts/sizes.

Rationale

The Guidelines recommend using a signed type for subscripts/indices. See ES.100 through ES.107. C++ already uses signed integers for array subscripts.

We want to be able to teach people to write "new clean modern code" that is simple, natural, warning-free at high warning levels, and doesn’t make us write a "pitfall" footnote about simple code.

If we don’t have a short adoptable word like index that is competitive with int and auto, people will still use int and auto and get their bugs. For example, they will write for(int i=0; i<v.size(); ++i) or for(auto i=0; i<v.size(); ++i) which have 32-bit size bugs on widely used platforms, and for(auto i=v.size()-1; i>=0; ++i) which just doesn't work. I don’t think we can teach for(ptrdiff_t i = ... with a straight face, or that people would accept it.

If we had a saturating arithmetic type, we might use that.
Otherwise, the best option is ptrdiff_t which has nearly all the advantages of a saturating arithmetic unsigned type, except only that ptrdiff_t still makes the pervasive loop style for(ptrdiff_t i=0; i<v.size(); ++i) emit signed/unsigned mismatches on i<v.size() (and similarly for i!=v.size()) for today's STL containers. (If a future STL changes its size_type to be signed, even this last drawback goes away.)

However, it would be hopeless (and embarrassing) to try to teach people to routinely write for (ptrdiff_t i = ... ; ... ; ...). (Even the Guidelines currently use it in only one place, and that's a "bad" example that is unrelated to indexing`.)

Therefore we should provide gsl::index (which can later be proposed for consideration as std::index) as a typedef for ptrdiff_t, so we can hopefully (and not embarrassingly) teach people to routinely write for (index i = ... ; ... ; ...).

Q&A

Why not just tell people to write ptrdiff_t? Because we believe it would be embarrassing to tell people that's what you have to do in C++, and even if we did people won't do it. Writing ptrdiff_t is too ugly and unadoptable compared to auto and int. The point of adding the name index is to make it as easy and attractive as possible to use a correctly sized signed type.

Should it be called index_t, following the C convention for typedefs? Please no. We are competing with auto and int, both of which have pitfalls for the unwary, and the point of adding this common name is to make it as easy and attractive as possible to use routinely. The extra characters _t (one of which requires hitting Shift; seriously) are a significant uglification for this type that we want to be pervasively used, and I fear those two extra characters will cause needless impedance compared to auto and int.

Is ptrdiff_t big enough? Yes. Standard containers are already required to have no more elements than can be represented by ptrdiff_t, because subtracting two iterators must fit in a difference_type.

But is ptrdiff_t really big enough, if I have a built-in array of char or byte that is bigger than half the size of the memory address space and so has more elements than can be represented in a ptrdiff_t? Yes. C++ already uses signed integers for array subscripts. So use index as the default option for the vast majority of uses including all built-in arrays. (If you do encounter the extremely rare case of an array, or array-like type, that is bigger than half the address space and whose elements are sizeof(1), and you're careful about avoiding truncation issues, go ahead and use a size_t for indexes into that very special container only. Such beasts are very rare in practice, and when they do arise often won't be indexed directly by user code. For example, they typically arise in a memory manager that takes over system allocation and parcels out individual smaller allocations that its users use, or in an MPEG or similar which provides its own interface; in both cases the size_t should only be needed internally within the memory manager or the MPEG class implementation.)

Will this still leave noisy signed/unsigned mixed comparison warnings? Hopefully not. The guidance in this PR allow-lists comparisons between index (aka ptrdiff_t) and either sizeof or a container .size() for compatibility with the current standard language and library. (Thanks to Robert Douglas for pointing out the sizeof noisy case.)

Why not use a saturating unsigned type? Then those big arrays would work, and I wouldn't get any signed/unsigned comparison mismatch warnings even using today's standard containers. Because we don't have a general-purpose saturating type in the standard. If we get one, we can revisit and consider using it.

Should gsl::span be updated to use index? Yes. In fact, it effectively already does so because it uses ptrdiff_t. Once we add index, GSL should change all of its other uses of ptrdiff_t to be index for stylistic consistency.

And update examples throughout to use `index` as appropriate
@cubbimew
Copy link
Member

Travis CI needs an update to the dictionary, otherwise it thinks you're talking about triffids

Line 12495: ptrdiff -> triffid
Line 12584: ptrdiff -> triffid
Line 12816: ptrdiff -> triffid
Line 19907: ptrdiff -> triffid

Same reason as container `.size()` -- better backward compatibility with
the existing standard
@AndrewPardoe
Copy link
Contributor

I've added ptrdiff to the dictionary.

@@ -1,6 +1,6 @@
# <a name="main"></a>C++ Core Guidelines

December 29, 2017
January 1, 2018
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes a merge conflict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants