It seems to me that there are compiler bug in Clang and GCC which mis-optimize code combines Reversed() with MakeRange(). If we have code > for (auto i : Reversed(MakeRange(n))) { > std::cout << i << std::endl; > } we expect that it outputs [0, n) reversely. However, on GCC with -O1 or higher, it outputs n times of 0, and on Clang with -O2 or higher, it outputs a 0 and then (n-1) times of several weird numbers. Interestingly, if I compile that on Clang with -fsanitize=undefined, the output would be n times of weird numbers. If compile with -fsanitize=integer, the program outputs [1,n] reversely. What may help is compiling on Clang with -fsanitize=memory. It reports use-of-uninitialized-value error for MakeRange(). If I expand everything explicitly to: > auto range = MakeRange(n); > auto iter = range.begin(); > auto end = range.end(); > while (true) { > bool done = (iter == end); > std::cout << done << std::endl; > if (done) { > break; > } > auto i = *iter; > std::cout << i << std::endl; > ++iter; > } The memory sanitizer reports that error on the line "if (done) {", which doesn't make sense at all.

This problem is because that the current impl of ReverseIterator contains an undefined behavior that we returns a reference of a variable in the current stack frame.

Errr, EnumeratedRange has the same issue...

Comment on attachment 8624601 [details] [diff] [review] patch 1 - change return type of operator*() of several iterators, r=roc Review of attachment 8624601 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/EnumeratedRange.h @@ +24,1 @@ > #include "mozilla/IntegerTypeTraits.h" Flip these to preserve alphabetical ordering. ::: mfbt/ReverseIterator.h @@ +15,2 @@ > #include "mozilla/Attributes.h" > #include "mozilla/IteratorTraits.h" Alphabetize, again. @@ +31,5 @@ > template<typename Iterator> > MOZ_IMPLICIT ReverseIterator(const ReverseIterator<Iterator>& aOther) > : mCurrent(aOther.mCurrent) { } > > + auto operator*() const -> decltype(*DeclVal<IteratorT>()) I think you want to uUse this decltype as the type in the ValueType typedef above, right?

Comment on attachment 8624058 [details] [diff] [review] patch 2 - add static_assert to ensure MakeRange used with integer Review of attachment 8624058 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/IntegerRange.h @@ +190,5 @@ > detail::IntegerRange<IntType2> > MakeRange(IntType1 aBegin, IntType2 aEnd) > { > + static_assert(IsIntegral<IntType1>::value && IsIntegral<IntType2>::value, > + "Values must both be integral"); Lowercase "value" and "values" in these, as I believe compilers generally treat the assertion message as a sentence fragment.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13) > Comment on attachment 8624601 [details] [diff] [review] > patch 1 - change return type of operator*() of several iterators, r=roc > > Review of attachment 8624601 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mfbt/EnumeratedRange.h > @@ +24,1 @@ > > #include "mozilla/IntegerTypeTraits.h" > > Flip these to preserve alphabetical ordering. > > ::: mfbt/ReverseIterator.h > @@ +15,2 @@ > > #include "mozilla/Attributes.h" > > #include "mozilla/IteratorTraits.h" > > Alphabetize, again. > > @@ +31,5 @@ > > template<typename Iterator> > > MOZ_IMPLICIT ReverseIterator(const ReverseIterator<Iterator>& aOther) > > : mCurrent(aOther.mCurrent) { } > > > > + auto operator*() const -> decltype(*DeclVal<IteratorT>()) > > I think you want to uUse this decltype as the type in the ValueType typedef > above, right? No. If we want a typedef for the return value of operator*(), it should be ReferenceType. But I don't think it helps a lot to introduce a new typedef here. Actually, I remove the useless ValueType and DifferenceType in patch 3.

Comment on attachment 8624603 [details] [diff] [review] patch 3 - remove unused operators and traits Review of attachment 8624603 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/ReverseIterator.h @@ -22,5 @@ > { > public: > - typedef typename IteratorTraits<IteratorT>::ValueType ValueType; > - typedef typename IteratorTraits<IteratorT>::DifferenceType DifferenceType; > - Ah, yes, I see you removed these later. That said, thinking slightly more, is there a reason you have to use auto -> syntax? Couldn't you just use |decltype(*DeclVal<IteratorT>()) operator*() const| to avoid the extra indirection of auto+trailing return type?

Comment on attachment 8624640 [details] [diff] [review] patch 4 - unittest for integer range Review of attachment 8624640 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/tests/TestIntegerRange.cpp @@ +5,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "mozilla/Assertions.h" > +#include "mozilla/IntegerRange.h" > +#include <cstdlib> What are you using from <cstdlib>? I don't see anything here except size_t and ptrdiff_t, maybe. But those are in <stddef.h>, and you should use that header to access them. Also, blank line before the standard-library include. @@ +7,5 @@ > +#include "mozilla/Assertions.h" > +#include "mozilla/IntegerRange.h" > +#include <cstdlib> > + > +using namespace mozilla; Policy is to only |using| specific symbols, not open up the entire namespace. Break this up: using mozilla::IsSame; using mozilla::MakeRange; using mozilla::Reversed; I think that's the full list, add others as needed. @@ +10,5 @@ > + > +using namespace mozilla; > + > +#define MAX_NUMBER 50 > +#define ARRAY_SIZE 256 const size_t for both of htese.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16) > That said, thinking slightly more, is there a reason you have to use auto -> > syntax? Couldn't you just use |decltype(*DeclVal<IteratorT>()) operator*() > const| to avoid the extra indirection of auto+trailing return type? Yes, if you prefer that style. I thought using auto -> syntax looks more clear, though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/13b68b2357f9 https://hg.mozilla.org/integration/mozilla-inbound/rev/addb5b055383 https://hg.mozilla.org/integration/mozilla-inbound/rev/685a87f8677c https://hg.mozilla.org/integration/mozilla-inbound/rev/61cae4c32db5

https://hg.mozilla.org/mozilla-central/rev/13b68b2357f9 https://hg.mozilla.org/mozilla-central/rev/addb5b055383 https://hg.mozilla.org/mozilla-central/rev/685a87f8677c https://hg.mozilla.org/mozilla-central/rev/61cae4c32db5