Closed Bug 1295611 Opened 8 years ago Closed 7 years ago

Add gsl::span-like mozilla::Span to MFBT

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Bug 1208262 (importing GSL itself) seems to have stalled. We already got gsl::not_null as mozilla::NotNull in bug 1272203.

We have mozilla::Range, but it stores a start pointer and a pointer past the end internally, which requires arithmetic when decomposing to pointer and length in order to pass through FFI to create the corresponding Rust slice from raw parts.

Also, the pre-existing classes that I'd like to amend to offer the ability view them as Spans, such as nsAString, nsACString and nsHTML5UTF16Buffer, already store a pointer and length (albeit the length isn't a size_t for legacy reasons).

Therefore, I suggest adding a mozilla::Span that wraps a pointer of type T (where T can include constness) and a size_t length based on the design of GSL's span: https://github.com/Microsoft/GSL/blob/master/gsl/span

I don't have use cases for multidimensional spans and I'm not sure if we need string_span in Gecko, so I'd consider this bug fixed without MFBT providing analogs for those parts of GSL.

Also, it would be nice to have a way to get a subspan with the Rust semantics for the indices even though GSL rejected that request: https://github.com/Microsoft/GSL/issues/270
The first use for this that I have in mind is for the inputs and outputs of encoding_rs when used from C++. I.e. to have an MFBT type wherever https://github.com/hsivonen/encoding_rs/blob/master/include/encoding_rs_cpp.h uses gsl::span. (The gsl::string_spans will become const nsACString&. Generalizing those to spans of some kind would be an overkill given our pre-existing patterns of use.)
Just to be clear here, because my newsgroup post wasn't: I am a fan of Range/Span and anything that gets the concept to be more widely used in Gecko gets my stamp of approval.
Summary: gsl::span-like mozilla::Span to MFBT → Add gsl::span-like mozilla::Span to MFBT
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Looking at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8713f5017e7b11d0b66123ea46fbc04a8ae6957c&selectedJob=77910998 it seems that Mac clang on tryserver is unhappy about single-argument constructors that are intentionally implicit. Locally, the code compiles without even warnings on Ubuntu 16.04's clang 3.8.

Any idea why clang doesn't like implicit constructors on Mac on try but is OK with them locally on Ubuntu? Is our Mac clang old?
Flags: needinfo?(nfroyd)
(In reply to Henri Sivonen (:hsivonen) from comment #3)
> Any idea why clang doesn't like implicit constructors on Mac on try but is
> OK with them locally on Ubuntu? Is our Mac clang old?

That job is enabling our clang plugin for static analysis, which wants all single argument constructors to be `explicit` or explicitly implicit with MOZ_IMPLICIT.  Assuming that you're OK with the implicit conversions, you'll want to tag all those constructors as MOZ_IMPLICIT:

  constexpr MOZ_IMPLICIT span(std::nullptr_t) GSL_NOEXCEPT : span() {}

and so forth.
Flags: needinfo?(nfroyd)
Attached patch WIP (obsolete) — Splinter Review
Does the handling of license headers look OK here? Should I add some Mozilla-specific legalese considering that this isn't verbatim from Microsoft but modified by me?

Upstream is https://github.com/Microsoft/GSL/
Attachment #8838508 - Flags: feedback?(gerv)
Comment on attachment 8838508 [details] [diff] [review]
WIP

LGTM.

Gerv
Attachment #8838508 - Flags: feedback?(gerv) → feedback+
Attached patch Add mozilla::Span (obsolete) — Splinter Review
(In reply to Gervase Markham [:gerv] from comment #6)
> LGTM.

Thanks.
Attachment #8838508 - Attachment is obsolete: true
Attachment #8840458 - Flags: review?(nfroyd)
Comment on attachment 8840458 [details] [diff] [review]
Add mozilla::Span

Review of attachment 8840458 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/license.html
@@ +3357,5 @@
>      <hr>
>  
> +    <h1><a id="gsl"></a>GSL License</h1>
> +
> +    <p>This license applies to <span class="path">mfbt/Byte.h</span> and

Oops. This should say Span.h.
Attachment #8840458 - Attachment is obsolete: true
Attachment #8840458 - Flags: review?(nfroyd)
Attachment #8841005 - Flags: review?(nfroyd)
Comment on attachment 8841005 [details] [diff] [review]
Add mozilla::Span, with license.html and formatting fixes

Review of attachment 8841005 [details] [diff] [review]:
-----------------------------------------------------------------

I will be around very intermittently for the next week or so, so if you're under a time constraint to get this in, you may want to find somebody else for second and subsequent passes over this.  I'm not opposed to something like this going in, but I'd like to talk through some of the stuff below.

I'm unsure how much you're willing to deviate from the original; I see you've written some nice block comments on Span usage, which is appreciated, but I think we could stand to trim things down a bit.

::: mfbt/Span.h
@@ +16,5 @@
> +
> +// Adapted from https://github.com/Microsoft/GSL/blob/3819df6e378ffccf0e29465afe99c3b324c2aa70/include/gsl/span
> +// and https://github.com/Microsoft/GSL/blob/3819df6e378ffccf0e29465afe99c3b324c2aa70/include/gsl/gsl_util
> +
> +#pragma once

Let's remove this, since we don't use #pragma once in Gecko.

@@ +59,5 @@
> +#pragma push_macro("constexpr")
> +#define constexpr /*constexpr*/
> +
> +// VS 2013 workarounds
> +#if _MSC_VER <= 1800

Since we don't support VS 2013, we should just remove 2013 workarounds.

@@ +84,5 @@
> +#ifdef GSL_THROW_ON_CONTRACT_VIOLATION
> +#define GSL_NOEXCEPT /*noexcept*/
> +#else
> +#define GSL_NOEXCEPT noexcept
> +#endif // GSL_THROW_ON_CONTRACT_VIOLATION

Can we just get rid of this and make everything noexcept (or not even declare noexcept), since we don't want things throwing exceptions?

@@ +108,5 @@
> +{
> +};
> +}
> +
> +// narrow() : a checked version of narrow_cast() that throws if the cast changed the value

Isn't this just the same as AssertedCast from mfbt/Casting.h (albeit with MOZ_RELEASE_ASSERT instead of MOZ_ASSERT)?

@@ +137,5 @@
> +namespace span_details {
> +
> +// C++14 types that we don't have because we build as C++11. Can't polyfill
> +// these into the std namespace, either, because clang static analysis on
> +// Windows has these in <type_traits>.

Should we just be using the versions in mozilla/TypeTraits.h?  <type_traits> has been seen to cause issues with our system header wrapping machinery, and I'd kinda like to use our versions until we can use <type_traits> without any problems.

@@ +230,5 @@
> +  operator=(const span_iterator<Span, IsConst>&) GSL_NOEXCEPT = default;
> +
> +  MOZ_SPAN_GCC_CONSTEXPR reference operator*() const
> +  {
> +    MOZ_RELEASE_ASSERT(span_);

Why are we release-asserting on the presence of `span_`?  Oh, so you're not iterating on null Spans?  I wish C++ iterators were less awful. =/

I am uneasy about Span's ability to bypass bounds-checking on nsTArray and related containers.

@@ +435,5 @@
> + * including (pre-decay) C arrays, XPCOM strings, nsTArray, mozilla::Array,
> + * mozilla::Range and contiguous standard-library containers, auto-convert
> + * into Spans when attempting to pass them as arguments to methods that take
> + * Spans. MakeSpan() functions can be used for explicit conversion in other
> + * contexts. (Span itself autoconverts into mozilla::Range.)

Did you keep the two things separate to avoid turning this into a mega-patch, or because `range' means something different in C++17 (C++20?)?  It would be nice to not have two-different-but-very-close things in MFBT.

@@ +461,5 @@
> + * has a method called Subspan() that works analogously to the Substring()
> + * method of XPCOM strings taking a start index and an optional length. As a
> + * Mozilla extension (relative to Microsoft's gsl::span that mozilla::Span is
> + * based on), Span has methods To(start), From(end) and FromTo(start, end)
> + * that correspond to Rust's &slice[start..], &slice[..end] and

The first two methods here seem backwardly named to me: From(x) sounds like you're starting from x and going until the end, but the documentation says it's the other way around, which seems confusing.  FromTo reads nicely: From start To end, but the single-argument From and To then take arguments opposite of how you might expect them to be taken in FromTo.

@@ +600,5 @@
> +  /**
> +   * Constructor for a single-element Span from std::shared_ptr.
> +   */
> +  constexpr MOZ_IMPLICIT Span(const std::shared_ptr<ElementType>& aPtr)
> +    : storage_(aPtr.get(), aPtr.get() ? 1 : 0)

Is there here for completeness, conformance with gsl::span, or something else?  I'm a bit on the fence with introducing shared_ptr concepts into the codebase more widely.

@@ +607,5 @@
> +
> +  // NB: the SFINAE here uses .data() as a incomplete/imperfect proxy for the requirement
> +  // on Container to be a contiguous sequence container.
> +  /**
> +   * Constructor for standard-library containers.

Given that we'd prefer people not use standard library containers in Gecko, I'm unenthusiastic about making Span interoperable with them.

@@ +1009,5 @@
> +template<class ElementType,
> +         size_t Extent,
> +         class = span_details::enable_if_t<!std::is_const<ElementType>::value>>
> +Span<uint8_t, span_details::calculate_byte_size<ElementType, Extent>::value>
> +AsWriteableBytes(Span<ElementType, Extent> s) GSL_NOEXCEPT

I think we should just ditch the Microsoft spelling to avoid confusion.

@@ +1015,5 @@
> +  return { reinterpret_cast<uint8_t*>(s.data()), s.size_bytes() };
> +}
> +
> +/**
> + * View span as Span<uint8_t> (common spelling).

This comment would then need to be updated.

@@ +1109,5 @@
> +  return Span<typename Ptr::element_type>(aPtr, aLength);
> +}
> +
> +/**
> + * Create one-element span from smart pointer.

I'm a bit skeptical that this is going to be used.

::: mfbt/tests/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +if not CONFIG['JS_STANDALONE']:
> +    TEST_DIRS += [
> +        'gtest',

Is there a philosophical reason that TestSpan isn't a simple program, like the rest of the MFBT tests?  Standalone JS builds/distributions won't test Span otherwise, which doesn't seem great.

::: xpcom/ds/nsTArray.h
@@ +1236,5 @@
>    const_reverse_iterator crend() const { return rend(); }
>  
> +  // Span integration
> +
> +  operator mozilla::Span<elem_type>()

Why have both the implicit conversion method(s) and MakeSpan overloads?  Why not one or the other?
Attachment #8841005 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj][high latency until 6 March] from comment #16)
> > +#pragma once
> 
> Let's remove this, since we don't use #pragma once in Gecko.

OK.

> @@ +59,5 @@
> > +#pragma push_macro("constexpr")
> > +#define constexpr /*constexpr*/
> > +
> > +// VS 2013 workarounds
> > +#if _MSC_VER <= 1800
> 
> Since we don't support VS 2013, we should just remove 2013 workarounds.

OK.

> @@ +84,5 @@
> > +#ifdef GSL_THROW_ON_CONTRACT_VIOLATION
> > +#define GSL_NOEXCEPT /*noexcept*/
> > +#else
> > +#define GSL_NOEXCEPT noexcept
> > +#endif // GSL_THROW_ON_CONTRACT_VIOLATION
> 
> Can we just get rid of this and make everything noexcept (or not even
> declare noexcept), since we don't want things throwing exceptions?

OK.

> @@ +108,5 @@
> > +{
> > +};
> > +}
> > +
> > +// narrow() : a checked version of narrow_cast() that throws if the cast changed the value
> 
> Isn't this just the same as AssertedCast from mfbt/Casting.h (albeit with
> MOZ_RELEASE_ASSERT instead of MOZ_ASSERT)?

Looks like it, yes.

> @@ +137,5 @@
> > +namespace span_details {
> > +
> > +// C++14 types that we don't have because we build as C++11. Can't polyfill
> > +// these into the std namespace, either, because clang static analysis on
> > +// Windows has these in <type_traits>.
> 
> Should we just be using the versions in mozilla/TypeTraits.h? 

I guess we should. I failed to notice that TypeTraits.h had them, because I was scanning for Mozilla-style name munging of the C++14 names but the naming in TypeTraits.h doesn't follow the C++14 naming.

> <type_traits>
> has been seen to cause issues with our system header wrapping machinery, and
> I'd kinda like to use our versions until we can use <type_traits> without
> any problems.

I was unaware of this.

> @@ +230,5 @@
> > +  operator=(const span_iterator<Span, IsConst>&) GSL_NOEXCEPT = default;
> > +
> > +  MOZ_SPAN_GCC_CONSTEXPR reference operator*() const
> > +  {
> > +    MOZ_RELEASE_ASSERT(span_);
> 
> Why are we release-asserting on the presence of `span_`?  Oh, so you're not
> iterating on null Spans?  I wish C++ iterators were less awful. =/

The assertions are where Microsoft put them.

> I am uneasy about Span's ability to bypass bounds-checking on nsTArray and
> related containers.

Do you mean by taking iterators by means other than C++11 ranged loops?

> @@ +435,5 @@
> > + * including (pre-decay) C arrays, XPCOM strings, nsTArray, mozilla::Array,
> > + * mozilla::Range and contiguous standard-library containers, auto-convert
> > + * into Spans when attempting to pass them as arguments to methods that take
> > + * Spans. MakeSpan() functions can be used for explicit conversion in other
> > + * contexts. (Span itself autoconverts into mozilla::Range.)
> 
> Did you keep the two things separate to avoid turning this into a
> mega-patch, or because `range' means something different in C++17 (C++20?)? 
> It would be nice to not have two-different-but-very-close things in MFBT.

I'm adding Span instead of changing Range, because:
 1) Span's storage (pointer and length as opposed to pointer and pointer) matches Rust's slices without arithmetic, so they decompose and recompose as the other on the other side of the FFI without arithmetic.
 2) An earlier dev-platform thread seemed to OK Span on the grounds of it having enough of a pedigree from C++ Core Guidelines.

I didn't remove Range, because:
 1) I didn't want to write the megapatch.
 2) I didn't want to step on the toes of people who use Range.
 3) I didn't want to slow down the landing of Span over concerns that Span might for some use case be worse than Range.

> @@ +461,5 @@
> > + * has a method called Subspan() that works analogously to the Substring()
> > + * method of XPCOM strings taking a start index and an optional length. As a
> > + * Mozilla extension (relative to Microsoft's gsl::span that mozilla::Span is
> > + * based on), Span has methods To(start), From(end) and FromTo(start, end)
> > + * that correspond to Rust's &slice[start..], &slice[..end] and
> 
> The first two methods here seem backwardly named to me: From(x) sounds like
> you're starting from x and going until the end,

That's what I intended.

> but the documentation says
> it's the other way around, which seems confusing.

Oops. The docs on the methods themselves are right, but the doc quoted above is wrong.

> @@ +600,5 @@
> > +  /**
> > +   * Constructor for a single-element Span from std::shared_ptr.
> > +   */
> > +  constexpr MOZ_IMPLICIT Span(const std::shared_ptr<ElementType>& aPtr)
> > +    : storage_(aPtr.get(), aPtr.get() ? 1 : 0)
> 
> Is there here for completeness, conformance with gsl::span, or something
> else?  I'm a bit on the fence with introducing shared_ptr concepts into the
> codebase more widely.

Making single-element spans from smart pointers is something that gsl::span does, so I retained it. I agree that it's questionable behavior.

Should I remove the bits that create single-element spans from smart pointers?

> @@ +607,5 @@
> > +
> > +  // NB: the SFINAE here uses .data() as a incomplete/imperfect proxy for the requirement
> > +  // on Container to be a contiguous sequence container.
> > +  /**
> > +   * Constructor for standard-library containers.
> 
> Given that we'd prefer people not use standard library containers in Gecko,
> I'm unenthusiastic about making Span interoperable with them.

We do have imported Google-originating code that uses standard-library containers. Isn't it good to make it easier for that kind of code to call into Mozilla-developed code (if Mozilla-developed code takes Spans)?

> @@ +1009,5 @@
> > +template<class ElementType,
> > +         size_t Extent,
> > +         class = span_details::enable_if_t<!std::is_const<ElementType>::value>>
> > +Span<uint8_t, span_details::calculate_byte_size<ElementType, Extent>::value>
> > +AsWriteableBytes(Span<ElementType, Extent> s) GSL_NOEXCEPT
> 
> I think we should just ditch the Microsoft spelling to avoid confusion.

OK.

> @@ +1015,5 @@
> > +  return { reinterpret_cast<uint8_t*>(s.data()), s.size_bytes() };
> > +}
> > +
> > +/**
> > + * View span as Span<uint8_t> (common spelling).
> 
> This comment would then need to be updated.

OK.

> @@ +1109,5 @@
> > +  return Span<typename Ptr::element_type>(aPtr, aLength);
> > +}
> > +
> > +/**
> > + * Create one-element span from smart pointer.
> 
> I'm a bit skeptical that this is going to be used.

Again, I just kept the gsl::span features that were there. I'd be OK with removing this if you think it should be removed.

> ::: mfbt/tests/moz.build
> @@ +5,5 @@
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> > +if not CONFIG['JS_STANDALONE']:
> > +    TEST_DIRS += [
> > +        'gtest',
> 
> Is there a philosophical reason that TestSpan isn't a simple program, like
> the rest of the MFBT tests?

I read some doc that said that no more such tests should be added and new compiled-code tests should be gtests. (In fact, I initially ported the test into a program analogous to the existing MFBT tests, but then I thought it wouldn't pass review any longer, so I ported it to gtest.)

> Standalone JS builds/distributions won't test
> Span otherwise, which doesn't seem great.

But surely we can trust Span to work if we run the tests in full-Gecko builds.

> ::: xpcom/ds/nsTArray.h
> @@ +1236,5 @@
> >    const_reverse_iterator crend() const { return rend(); }
> >  
> > +  // Span integration
> > +
> > +  operator mozilla::Span<elem_type>()
> 
> Why have both the implicit conversion method(s) and MakeSpan overloads?  Why
> not one or the other?

1) Because gsl::span has both.
2) Because the implicit conversions are ergonomic when passing things that autoconvert to Span as arguments to methods that take Spans, but MakeSpan is more ergonomic when making a local Span variable out of something else without having to write out the type parameters, that is auto foo = MakeSpan(bar) (or when for whatever reason C++ doesn't invoke the implicit conversion).
Comment on attachment 8844858 [details] [diff] [review]
Add mozilla::Span, without single-element conversions from smart pointers and using mfbt's TypeTraits.h

Review of attachment 8844858 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the edits; we're getting closer.

::: mfbt/Span.h
@@ +24,5 @@
> +#include "mozilla/UniquePtr.h"
> +#include "mozilla/Array.h"
> +#include "mozilla/IntegerTypeTraits.h"
> +#include "mozilla/Casting.h"
> +#include "mozilla/TypeTraits.h"

MFBT style is to sort these alphabetically.

@@ +95,5 @@
> +template<bool B, class T = void>
> +using enable_if_t = typename mozilla::EnableIf<B, T>::Type;
> +
> +template<class T>
> +struct is_span_oracle : std::false_type

We should use mozilla::FalseType/TrueType here and throughout.

@@ +126,5 @@
> +};
> +
> +template<size_t From, size_t To>
> +struct is_allowed_extent_conversion
> +  : public std::integral_constant<bool,

mozilla::IntegralConstant, perhaps?  <type_traits> is not included in this file, and has issues as noted previously.

@@ +136,5 @@
> +
> +template<class From, class To>
> +struct is_allowed_element_type_conversion
> +  : public std::
> +      integral_constant<bool, std::is_convertible<From (*)[], To (*)[]>::value>

mozilla::IntegralConstant, perhaps?  <type_traits> is not included in this file, and has issues as noted previously.  We also have mozilla::IsConvertible to use here.

@@ +229,5 @@
> +    return *this;
> +  }
> +
> +  MOZ_SPAN_NON_CONST_CONSTEXPR span_iterator
> +  operator-(difference_type n) const

This is sort of an odd place for MOZ_SPAN_NON_CONST_CONSTEXPR; by symmetry with operator+, shouldn't this be MOZ_SPAN_CONSTEXPR_NOT_JUST_RETURN?

@@ +312,5 @@
> +
> +template<class Span, bool IsConst>
> +inline constexpr span_iterator<Span, IsConst>
> +operator-(typename span_iterator<Span, IsConst>::difference_type n,
> +          const span_iterator<Span, IsConst>& rhs)

This function seems sort of weird...we're saying that you can write:

  auto i = span.begin()
  return 5 - i;

and it does sensible things?  That seems wrong.

@@ +434,5 @@
> +    span_details::span_iterator<Span<ElementType, Extent>, false>;
> +  using const_iterator =
> +    span_details::span_iterator<Span<ElementType, Extent>, true>;
> +  using reverse_iterator = std::reverse_iterator<iterator>;
> +  using const_reverse_iterator = std::reverse_iterator<const_iterator>;

We can either use mozilla::ReverseIterator for this, or we need to include <iterator> in this header.

@@ +694,5 @@
> +    index_type aLength = dynamic_extent) const
> +  {
> +    MOZ_RELEASE_ASSERT((aStart == 0 || (aStart > 0 && aStart <= size())) &&
> +                       (aLength == dynamic_extent ||
> +                        (aLength >= 0 && aStart + aLength <= size())));

I'm surprised nothing complains that aLength >= 0 is always true, here and in a lot of similar places.

Isn't (aStart == 0 || (aStart > 0 && aStart <= size())) just (aStart <= size())?  Or are we writing something more complex (as happens elsewhere) because we want to ensure that if index_type is signed, our asserts still work as expected?  Maybe we should just static_assert unsignedness of index_type and simplify things quite a bit.

aStart + aLength <= size() is not quite a complete assertion, because you also want to ensure that aStart + aLength didn't wrap.  Not sure if you actually want to handle that here, given that this expression is already pretty complicated.

@@ +853,5 @@
> +{
> +#if 1
> +  return (l.size() == r.size()) && std::equal(l.begin(), l.end(), r.begin());
> +#else
> +  return std::equal(l.begin(), l.end(), r.begin(), r.end());

Can we remove this dead code and the corresponding preprocessing conditionals?

Also, I think you want to #include <algorithm> above if you're going to use std::equal and std::lexicographical_compare.

@@ +906,5 @@
> +// constexpr
> +// and so will fail compilation of the template
> +template<class ElementType, size_t Extent>
> +struct calculate_byte_size
> +  : std::integral_constant<size_t,

mozilla::IntegralConstant, perhaps?  <type_traits> is not included in this file, and has issues as noted previously.

::: mfbt/tests/gtest/TestSpan.cpp
@@ +30,5 @@
> +#include <map>
> +#include <memory>
> +#include <string>
> +#include <vector>
> +#include <regex>

We don't have regex tests enabled, maybe we should delete it...and possibly other headers as well?

::: toolkit/content/license.html
@@ +3421,5 @@
> +    <h1><a id="gsl"></a>GSL License</h1>
> +
> +    <p>This license applies to <span class="path">mfbt/Span.h</span> and
> +    <span class="path">mfbt/tests/gtest/TestSpan.cpp</span>.</p>
> +    <!-- https://github.com/Microsoft/GSL/blob/3819df6e378ffccf0e29465afe99c3b324c2aa70/LICENSE -->

You'll need to get license review from Gerv on this bit.
Attachment #8844858 - Flags: review?(nfroyd) → feedback+
(Moved to mozreview.)

The new patch introduces constructor and MakeSpan from const char*. The patch that I'm building on top of this one showed a use case for that.

(In reply to Nathan Froyd [:froydnj] from comment #20)
> ::: mfbt/Span.h
> @@ +24,5 @@
> > +#include "mozilla/UniquePtr.h"
> > +#include "mozilla/Array.h"
> > +#include "mozilla/IntegerTypeTraits.h"
> > +#include "mozilla/Casting.h"
> > +#include "mozilla/TypeTraits.h"
> 
> MFBT style is to sort these alphabetically.

Done.

> @@ +95,5 @@
> > +template<bool B, class T = void>
> > +using enable_if_t = typename mozilla::EnableIf<B, T>::Type;
> > +
> > +template<class T>
> > +struct is_span_oracle : std::false_type
> 
> We should use mozilla::FalseType/TrueType here and throughout.

Done.

> @@ +126,5 @@
> > +};
> > +
> > +template<size_t From, size_t To>
> > +struct is_allowed_extent_conversion
> > +  : public std::integral_constant<bool,
> 
> mozilla::IntegralConstant, perhaps?

Done.

> @@ +136,5 @@
> > +
> > +template<class From, class To>
> > +struct is_allowed_element_type_conversion
> > +  : public std::
> > +      integral_constant<bool, std::is_convertible<From (*)[], To (*)[]>::value>
> 
> mozilla::IntegralConstant, perhaps?  <type_traits> is not included in this
> file, and has issues as noted previously.  We also have
> mozilla::IsConvertible to use here.

Done.

> @@ +229,5 @@
> > +    return *this;
> > +  }
> > +
> > +  MOZ_SPAN_NON_CONST_CONSTEXPR span_iterator
> > +  operator-(difference_type n) const
> 
> This is sort of an odd place for MOZ_SPAN_NON_CONST_CONSTEXPR; by symmetry
> with operator+, shouldn't this be MOZ_SPAN_CONSTEXPR_NOT_JUST_RETURN?

I just ripped out constexpr and did another round of compilation. I can't recall why these two ended up being removed in different rounds. Anyway, I changed the above case to MOZ_SPAN_CONSTEXPR_NOT_JUST_RETURN.

> @@ +312,5 @@
> > +
> > +template<class Span, bool IsConst>
> > +inline constexpr span_iterator<Span, IsConst>
> > +operator-(typename span_iterator<Span, IsConst>::difference_type n,
> > +          const span_iterator<Span, IsConst>& rhs)
> 
> This function seems sort of weird...we're saying that you can write:
> 
>   auto i = span.begin()
>   return 5 - i;
> 
> and it does sensible things?  That seems wrong.

I blame Microsoft. What do you suggest be done?

> @@ +434,5 @@
> > +    span_details::span_iterator<Span<ElementType, Extent>, false>;
> > +  using const_iterator =
> > +    span_details::span_iterator<Span<ElementType, Extent>, true>;
> > +  using reverse_iterator = std::reverse_iterator<iterator>;
> > +  using const_reverse_iterator = std::reverse_iterator<const_iterator>;
> 
> We can either use mozilla::ReverseIterator for this, or we need to include
> <iterator> in this header.

Included <iterator>.

> @@ +694,5 @@
> > +    index_type aLength = dynamic_extent) const
> > +  {
> > +    MOZ_RELEASE_ASSERT((aStart == 0 || (aStart > 0 && aStart <= size())) &&
> > +                       (aLength == dynamic_extent ||
> > +                        (aLength >= 0 && aStart + aLength <= size())));
> 
> I'm surprised nothing complains that aLength >= 0 is always true, here and
> in a lot of similar places.
> 
> Isn't (aStart == 0 || (aStart > 0 && aStart <= size())) just (aStart <=
> size())?  Or are we writing something more complex (as happens elsewhere)
> because we want to ensure that if index_type is signed, our asserts still
> work as expected?

Microsoft's index_type was signed. I removed the traces of signed index_type.

> aStart + aLength <= size() is not quite a complete assertion, because you
> also want to ensure that aStart + aLength didn't wrap.  Not sure if you
> actually want to handle that here, given that this expression is already
> pretty complicated.

Left it unhandled.

> @@ +853,5 @@
> > +{
> > +#if 1
> > +  return (l.size() == r.size()) && std::equal(l.begin(), l.end(), r.begin());
> > +#else
> > +  return std::equal(l.begin(), l.end(), r.begin(), r.end());
> 
> Can we remove this dead code and the corresponding preprocessing
> conditionals?

Done.
 
> Also, I think you want to #include <algorithm> above if you're going to use
> std::equal and std::lexicographical_compare.

Done.

> @@ +906,5 @@
> > +// constexpr
> > +// and so will fail compilation of the template
> > +template<class ElementType, size_t Extent>
> > +struct calculate_byte_size
> > +  : std::integral_constant<size_t,
> 
> mozilla::IntegralConstant, perhaps?

Done.

> ::: mfbt/tests/gtest/TestSpan.cpp
> @@ +30,5 @@
> > +#include <map>
> > +#include <memory>
> > +#include <string>
> > +#include <vector>
> > +#include <regex>
> 
> We don't have regex tests enabled, maybe we should delete it...and possibly
> other headers as well?

Removed them all.

> ::: toolkit/content/license.html
> @@ +3421,5 @@
> > +    <h1><a id="gsl"></a>GSL License</h1>
> > +
> > +    <p>This license applies to <span class="path">mfbt/Span.h</span> and
> > +    <span class="path">mfbt/tests/gtest/TestSpan.cpp</span>.</p>
> > +    <!-- https://github.com/Microsoft/GSL/blob/3819df6e378ffccf0e29465afe99c3b324c2aa70/LICENSE -->
> 
> You'll need to get license review from Gerv on this bit.

I'll ask Gerv to take another look once the patch is technically ready.

Thank you.
Comment on attachment 8851987 [details]
Bug 1295611 - Add mozilla::Span.

https://reviewboard.mozilla.org/r/124244/#review126858

I'm skeptical that we're going to find all these `Span` conversions sprinkled throughout useful, but meh.

Some comments on the new `MakeSpan` and supporting infrastructure below.  I think that's the only remaining issue.

::: mfbt/Span.h:318
(Diff revision 1)
> +operator-(typename span_iterator<Span, IsConst>::difference_type n,
> +          const span_iterator<Span, IsConst>& rhs)

I think we should just remove this function.

::: mfbt/Span.h:574
(Diff revision 1)
> +  /**
> +   * Constructor for C string.
> +   */
> +  constexpr Span(const char* aPtr)
> +    : storage_(aPtr, std::strlen(aPtr))
> +  {
> +  }

This seems convenient, but I'm skeptical that you want such a constructor for arbitrary `T`: it opens up many footgun possibilities.  I think this constructor should be removed.

::: mfbt/Span.h:1023
(Diff revision 1)
> +inline Span<const char>
> +MakeSpan(const char* aStr)
> +{
> +  return Span<const char>(aStr);
> +}

I think the preferred path here, given the problems with the additional `Span` constructor, is to call this `MakeStringSpan` or `MakeCStringSpan`, and have the call to `strlen` be here, rather than hidden inside the constructor's initialization list.

Thanks for adding tests for this function.
Attachment #8851987 - Flags: review?(nfroyd)
Comment on attachment 8851987 [details]
Bug 1295611 - Add mozilla::Span.

https://reviewboard.mozilla.org/r/124244/#review126858

> This seems convenient, but I'm skeptical that you want such a constructor for arbitrary `T`: it opens up many footgun possibilities.  I think this constructor should be removed.

Yeah, having it for arbitrary `T` is bad. First, I tried to enable this constructor only for `Span<const char>`, but I'm not properly familiar with how exactly `enable_if_t` is supposed to interact with the syntax around it, so I failed. Then I figured I'd just leave it like this and let compile errors show up when it's used the wrong way, but then I realized that the current state is bad, because it breaks overloads for multiple types of spans, e.g. `Foo(Span<const char>)` and `Foo(Span<const char16_t>)`.

I'd still really like to have this constructor for `Span<const char>`, because it would be convenient to be able to write a single method that takes `Span<const char>` and be able to pass `nsACString`, `std::string` (yes, occurs in the spell checker code) or `const char*` (sadly, occurs in old XPCOM code more than one would hope) without extra ceremony.

Would you be OK with having this constructor for `Span<const char>` only? If yes, can you point me to the right direction for how to use `mozilla::EnableIf` to accomplish that?
(needinfoing for the above comment just in case.)
Flags: needinfo?(nfroyd)
(In reply to Henri Sivonen (:hsivonen) from comment #25)
> Would you be OK with having this constructor for `Span<const char>` only? If
> yes, can you point me to the right direction for how to use
> `mozilla::EnableIf` to accomplish that?

I'm still not wild about cases like:

  extern void foo(Span<const char>);

  char* buffer = ...; // not null-terminated in any way
  foo(buffer); // boom!

We've tried to be fairly explicit about conversions (e.g. the static analysis for `explicit` constructors); while this patch adds a lot of them, they are typically safe conversions.  Adding this constructor makes it very easy to introduce an unsafe conversion, and I don't think it's unreasonable to ask that people using raw pointers jump through an extra hoop to call `foo` in the above example.  Callers of `foo` that want to pass nsCString or std::string can still get a hassle-free experience.
Flags: needinfo?(nfroyd)
(In reply to Henri Sivonen (:hsivonen) from comment #25)
> Would you be OK with having this constructor for `Span<const char>` only? If
> yes, can you point me to the right direction for how to use
> `mozilla::EnableIf` to accomplish that?

I think we can employ a trick similar to what you use for the default constructor:

template <class E = ElementType,
          class = span_details::enable_if_t<IsSame<E, const char>::value>>
Span(const char* aPtr);
(I'm not arguing for such a constructor, just mentioning the technique for general reference.)
(In reply to Nathan Froyd [:froydnj] from comment #27)
> We've tried to be fairly explicit about conversions (e.g. the static
> analysis for `explicit` constructors); while this patch adds a lot of them,
> they are typically safe conversions.  Adding this constructor makes it very
> easy to introduce an unsafe conversion, and I don't think it's unreasonable
> to ask that people using raw pointers jump through an extra hoop to call
> `foo` in the above example.

OK. I'll remove the constructor and rename the MakeSpan to MakeCStringSpan.

(In reply to Botond Ballo [:botond] from comment #28)
> I think we can employ a trick similar to what you use for the default
> constructor:
> 
> template <class E = ElementType,
>           class = span_details::enable_if_t<IsSame<E, const char>::value>>
> Span(const char* aPtr);

Thank you. (The class E = ElementType bit was what I was missing.) I'll do what Nathan said, however.
Comment on attachment 8851987 [details]
Bug 1295611 - Add mozilla::Span.

https://reviewboard.mozilla.org/r/124242/#review127614

Placeholder comment to hopefully make ReviewBoard accept the re-review request.
Comment on attachment 8851987 [details]
Bug 1295611 - Add mozilla::Span.

https://reviewboard.mozilla.org/r/124244/#review127688

Thank you!
Attachment #8851987 - Flags: review?(nfroyd) → review+
Comment on attachment 8851987 [details]
Bug 1295611 - Add mozilla::Span.

https://reviewboard.mozilla.org/r/124242/#review127696

Requesting review from Gerv for the licensing bits (which should be substantially the same as at the time of the previous feedback+).
Comment on attachment 8851987 [details]
Bug 1295611 - Add mozilla::Span.

It's not obvious to me how to r+ this in MozReview; I keep getting a message about "squashed commits", which says I need to review commits individually but the instructions on how to do that are unclear.

r=gerv.

Gerv
Attachment #8851987 - Flags: review?(gerv) → review+
(In reply to Gervase Markham [:gerv] from comment #37)
> Comment on attachment 8851987 [details]
> Bug 1295611 - Add mozilla::Span.
> 
> It's not obvious to me how to r+ this in MozReview; I keep getting a message
> about "squashed commits", which says I need to review commits individually
> but the instructions on how to do that are unclear.

Thanks for the r+. Did you try the "Finish review" UI? (If ReviewBoard doesn't believe you r+ed the patch, it won't let r=gerv appear in the commit message.)
Flags: needinfo?(gerv)
I pushed manually.
Flags: needinfo?(gerv)
https://hg.mozilla.org/mozilla-central/rev/261772d71985
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Out of curiosity, why is there `#if 0` code in the gtest? Do you have plans to enable these at some point? (Comments would have been helpful.)
Flags: needinfo?(hsivonen)
(In reply to David Major [:dmajor] from comment #42)
> Out of curiosity, why is there `#if 0` code in the gtest? Do you have plans
> to enable these at some point? (Comments would have been helpful.)

Those are Microsoft's tests that aren't applicable in the MFBT case at this time. I avoided throwing away test code to make it easier to resurrect it later if needed, but I don't have any plans to do so.
Flags: needinfo?(hsivonen)
Depends on: 1403083
You need to log in before you can comment on or make changes to this bug.