0027571: Bad implementation of NCollection_Sequence::Iterator

Reverse iterator after creation points to the first iterated item now, i.e. last item of sequence.
Test case was created.

src/NCollection/NCollection_BaseSequence.hxx

@@ -58,34 +58,40 @@ public:
   {
   public:
     //! Empty constructor
-    Iterator (void) : myCurrent (NULL), myPrevious(NULL) {}
+    Iterator (void) : myCurrent (NULL), myPrevious(NULL), myIsReversed(Standard_False) {}
 
-    //! Constructor with initialisation
+    //! Constructor with initialization
     Iterator (const NCollection_BaseSequence& theSeq,
               const Standard_Boolean isStart)
     {
       Init (theSeq, isStart);
     }
 
-     //! Initialisation
+     //! Initialization
     void Init (const NCollection_BaseSequence& theSeq,
                const Standard_Boolean isStart = Standard_True)
     {
-      myCurrent  = (isStart ? theSeq.myFirstItem : NULL);
-      myPrevious = (isStart ? NULL : theSeq.myLastItem);
+      myCurrent = (isStart ? theSeq.myFirstItem : theSeq.myLastItem);
+      myPrevious = NULL;
+      myIsReversed = !isStart;
     }
 
     //! Switch to previous element; note that it will reset 
     void Previous()
     {
       myCurrent = myPrevious;
-      if (myCurrent)
-        myPrevious = myCurrent->Previous();
+      if (myCurrent) {
+        if (!myIsReversed)
+          myPrevious = myCurrent->Previous();
+        else
+          myPrevious = myCurrent->Next();
+      }
     }
       
   protected:
     NCollection_SeqNode* myCurrent;  //!< Pointer to the current node
     NCollection_SeqNode* myPrevious; //!< Pointer to the previous node
+    Standard_Boolean myIsReversed;   //!< Flag to create reversed iterator
     friend class NCollection_BaseSequence;
   };
 

src/NCollection/NCollection_Sequence.hxx

@@ -71,7 +71,10 @@ public:
       if (myCurrent)
       {
         myPrevious = myCurrent;
-        myCurrent = myCurrent->Next();
+        if (myIsReversed)
+          myCurrent = myCurrent->Previous();
+        else
+          myCurrent = myCurrent->Next();
       }
     }
     //! Constant value access
@@ -93,19 +96,38 @@ public:
   //! Shorthand for a constant iterator type.
   typedef NCollection_StlIterator<std::bidirectional_iterator_tag, Iterator, TheItemType, true> const_iterator;
 
+  //! Shorthand for a reverse iterator type.
+  typedef NCollection_StlIterator<std::reverse_iterator<iterator>, Iterator, TheItemType, false> reverse_iterator;
+
+  //! Shorthand for a constant reverse iterator type.
+  typedef NCollection_StlIterator<std::reverse_iterator<const_iterator>, Iterator, TheItemType, true> const_reverse_iterator;
+
   //! Returns an iterator pointing to the first element in the sequence.
   iterator begin() const { return Iterator (*this, true); }
 
   //! Returns an iterator referring to the past-the-end element in the sequence.
-  iterator end() const { Iterator anIter (*this, false); anIter.Next(); return anIter; }
+  iterator end() const { Iterator anIter (*this, false); anIter.Previous(); return anIter; }
   
   //! Returns a const iterator pointing to the first element in the sequence.
   const_iterator cbegin() const { return Iterator (*this, true); }
 
   //! Returns a const iterator referring to the past-the-end element in the sequence.
-  const_iterator cend() const { Iterator anIter (*this, false); anIter.Next(); return anIter; }
+  const_iterator cend() const { Iterator anIter(*this, false); anIter.Previous(); return anIter; }
 
- public:
+  //! Returns a reverse iterator pointing to the last element in the sequence.
+  reverse_iterator rbegin() const { return Iterator(*this, false); }
+
+  //! Returns a reverse iterator referring to the past-the-end element in the reversed sequence.
+  reverse_iterator rend() const { Iterator anIter(*this, true); anIter.Previous(); return anIter; }
+
+  //! Returns a const reverse iterator pointing to the last element in the sequence.
+  const_reverse_iterator crbegin() const { return Iterator(*this, false); }
+
+  //! Returns a const reverse iterator referring to the past-the-end element in the reversed sequence.
+  const_reverse_iterator crend() const { Iterator anIter(*this, true); anIter.Previous(); return anIter; }
+
+
+public:
   // ---------- PUBLIC METHODS ------------
 
   //! Empty constructor.

src/NCollection/NCollection_StlIterator.hxx

@@ -134,7 +134,9 @@ public: //! @name methods related to bidirectional STL iterator
   NCollection_StlIterator& operator--()
   {
     Standard_STATIC_ASSERT((opencascade::std::is_same<std::bidirectional_iterator_tag,Category>::value ||
-                            opencascade::std::is_same<std::random_access_iterator_tag,Category>::value));
+                            opencascade::std::is_same<std::random_access_iterator_tag,Category>::value ||
+                            opencascade::std::is_same<std::reverse_iterator<NCollection_StlIterator<std::bidirectional_iterator_tag, BaseIterator, ItemType, false>>, Category>::value ||
+                            opencascade::std::is_same<std::reverse_iterator<NCollection_StlIterator<std::bidirectional_iterator_tag, BaseIterator, ItemType, true>>, Category>::value));
     myIterator.Previous();
     return *this;
   }

src/QABugs/QABugs_20.cxx

@@ -4359,7 +4359,74 @@ static Standard_Integer QACheckBends(Draw_Interpretor& theDI,
   return 0;
 }
 
+#include <NCollection_Sequence.hxx>
+static Standard_Integer OCC27571(Draw_Interpretor& di, Standard_Integer /*nb*/, const char ** /*a*/)
+{
+  // Create NCollection_Sequence<TCollection_AsciiString>
+  NCollection_Sequence<TCollection_AsciiString> aSequence;
+  aSequence.Append("1");
+  aSequence.Append("2");
+  aSequence.Append("3");
+  aSequence.Append("4");
+  aSequence.Append("5");
 
+  // Create iterator
+  NCollection_Sequence<TCollection_AsciiString>::Iterator anIterator(aSequence);
+
+  // Create reverse iterator
+  NCollection_Sequence<TCollection_AsciiString>::Iterator aReverseIterator(aSequence, Standard_False);
+
+  // Display elements using iterator (with More/Next)
+  di << "Iterator (More/Next):";
+  for (; anIterator.More(); anIterator.Next())
+    di << " " << anIterator.Value();
+  anIterator.Previous();
+  di << "\nCurrent (last) element - Iterator: " << anIterator.Value();
+  anIterator.Previous();
+  di << "\nPrevious element - Iterator: " << anIterator.Value();
+
+  // Display elements using reverse iterator (with More/Next)
+  di << "\nReverse iterator (More/Next):";
+  for (; aReverseIterator.More(); aReverseIterator.Next())
+    di << " " << aReverseIterator.Value();
+  aReverseIterator.Previous();
+  di << "\nCurrent (last) element - Reverse iterator: " << aReverseIterator.Value();
+  aReverseIterator.Previous();
+  di << "\nPrevious element - Reverse iterator: " << aReverseIterator.Value();
+
+  // Display elements using iterator (with begin/end)
+  di << "\nIterator (begin/end):";
+  NCollection_Sequence<TCollection_AsciiString>::iterator anIteratorBeginEnd = aSequence.begin();
+  for (; anIteratorBeginEnd != aSequence.end(); ++anIteratorBeginEnd)
+    di << " " << *anIteratorBeginEnd;
+  --anIteratorBeginEnd;
+  di << "\nCurrent (last) element - Iterator (begin/end): " << *anIteratorBeginEnd;
+
+  // Display elements using const_iterator (with cbegin/cend)
+  di << "\nIterator (cbegin/cend) Const:";
+  NCollection_Sequence<TCollection_AsciiString>::const_iterator anIteratorBeginEndConst = aSequence.cbegin();
+  for (; anIteratorBeginEndConst != aSequence.cend(); ++anIteratorBeginEndConst)
+    di << " " << *anIteratorBeginEndConst;
+  --anIteratorBeginEndConst;
+  di << "\nCurrent (last) element - Iterator (cbegin/cend) Const: " << *anIteratorBeginEndConst;
+
+  // Display elements using reverse_iterator (with rbegin/rend)
+  di << "\nReverse iterator (rbegin/rend):";
+  NCollection_Sequence<TCollection_AsciiString>::reverse_iterator aReverseIteratorBeginEnd = aSequence.rbegin();
+  for (; aReverseIteratorBeginEnd != aSequence.rend(); ++aReverseIteratorBeginEnd)
+    di << " " << *aReverseIteratorBeginEnd;
+  --aReverseIteratorBeginEnd;
+  di << "\nCurrent (last) element - Reverse iterator (rbegin/rend): " << *aReverseIteratorBeginEnd;
+
+  // Display elements using const_reverse_iterator (with crbegin/crend)
+  di << "\nReverse iterator (crbegin/crend) Const:";
+  NCollection_Sequence<TCollection_AsciiString>::const_reverse_iterator aReverseIteratorBeginEndConst = aSequence.crbegin();
+  for (; aReverseIteratorBeginEndConst != aSequence.crend(); ++aReverseIteratorBeginEndConst)
+    di << " " << *aReverseIteratorBeginEndConst;
+  --aReverseIteratorBeginEndConst;
+  di << "\nCurrent (last) element - Reverse iterator (crbegin/crend) Const: " << *aReverseIteratorBeginEndConst;
+  return 0;
+}
 
 void QABugs::Commands_20(Draw_Interpretor& theCommands) {
   const char *group = "QABugs";
@@ -4467,5 +4534,7 @@ void QABugs::Commands_20(Draw_Interpretor& theCommands) {
     __FILE__,
     QACheckBends, group);
 
+  theCommands.Add("OCC27571", "OCC27571", __FILE__, OCC27571, group);
+
   return;
 }

tests/bugs/fclasses/bug27571

@@ -0,0 +1,118 @@
+puts "========"
+puts "OCC27571"
+puts "========"
+puts ""
+########################################################
+# Bad implementation of NCollection_Sequence::Iterator
+########################################################
+
+pload QAcommands
+
+set info [OCC27571]
+
+puts ""
+
+set Sequence "1 2 3 4 5"
+set Sequence_Last "5"
+set Sequence_Preult "4"
+set ReversedSequence "5 4 3 2 1"
+set ReversedSequence_Last "1"
+set ReversedSequence_Preult "2"
+
+# Iterator using Init(aSequence, Standard_True) method.
+if { [regexp "Iterator \\(More/Next\\): ${Sequence}" $info] } {
+  puts "OK : Iterator (More/Next)"
+} else {
+  puts "Error : Iterator (More/Next)"
+}
+
+# Method Previous() called after full iteration, current value of Iterator is equal to last element of sequence
+if { [regexp "Current \\(last\\) element - Iterator: ${Sequence_Last}" $info] } {
+  puts "OK : Current (last) element - Iterator"
+} else {
+  puts "Error : Current (last) element - Iterator"
+}
+
+# Method Previous() called once more again, current value of Iterator is equal to penult element of sequence
+if { [regexp "Previous element - Iterator: ${Sequence_Preult}" $info] } {
+  puts "OK : Previous element - Iterator"
+} else {
+  puts "Error : Previous element - Iterator"
+}
+
+# Reverse iterator using Init(aSequence, Standard_False) method.
+if { [regexp "Reverse iterator \\(More/Next\\): ${ReversedSequence}" $info] } {
+  puts "OK : Reverse iterator (More/Next)"
+} else {
+  puts "Error : Reverse iterator (More/Next)"
+}
+
+# Method Previous() called after full iteration, current value of Iterator is equal to last element of sequence
+if { [regexp "Current \\(last\\) element - Reverse iterator: ${ReversedSequence_Last}" $info] } {
+  puts "OK : Current (last) element - Reverse iterator"
+} else {
+  puts "Error : Current (last) element - Reverse iterator"
+}
+
+# Method Previous() called once more again, current value of Iterator is equal to penult element of sequence
+if { [regexp "Previous element - Reverse iterator: ${ReversedSequence_Preult}" $info] } {
+  puts "OK : Previous element - Reverse iterator"
+} else {
+  puts "Error : Previous element - Reverse iterator"
+}
+
+# Iterator using method "begin".
+if { [regexp "Iterator \\(begin/end\\): ${Sequence}" $info] } {
+  puts "OK : Iterator (begin/end)"
+} else {
+  puts "Error : Iterator (begin/end)"
+}
+
+# Check previous element after full iteration, current value of Iterator is equal to last element of sequence
+if { [regexp "Current \\(last\\) element - Iterator \\(begin/end\\): ${Sequence_Last}" $info] } {
+  puts "OK : Current (last) element - Iterator (begin/end)"
+} else {
+  puts "Error : Current (last) element - Iterator (begin/end)"
+}
+
+# Const iterator using method "begin".
+if { [regexp "Iterator \\(cbegin/cend\\) Const: ${Sequence}" $info] } {
+  puts "OK : Iterator (cbegin/cend) Const"
+} else {
+  puts "Error : Iterator (cbegin/cend) Const"
+}
+
+# Check previous element after full iteration, current value of Iterator is equal to last element of sequence
+if { [regexp "Current \\(last\\) element - Iterator \\(cbegin/cend\\) Const: ${Sequence_Last}" $info] } {
+  puts "OK : Current (last) element - Iterator (cbegin/cend) Const"
+} else {
+  puts "Error : Current (last) element - Iterator (cbegin/cend) Const"
+}
+
+# Reverse iterator using method "rbegin".
+if { [regexp "Reverse iterator \\(rbegin/rend\\): ${ReversedSequence}" $info] } {
+  puts "OK : Reverse iterator (rbegin/rend)"
+} else {
+  puts "Error : Reverse iterator (rbegin/rend)"
+}
+
+# Check previous element after full iteration, current value of Iterator is equal to last element of sequence
+if { [regexp "Current \\(last\\) element - Reverse iterator \\(rbegin/rend\\): ${ReversedSequence_Last}" $info] } {
+  puts "OK : Current (last) element - Iterator (rbegin/rend)"
+} else {
+  puts "Error : Current (last) element - Iterator (rbegin/rend)"
+}
+
+# Const reverse iterator using method "crbegin".
+if { [regexp "Reverse iterator \\(crbegin/crend\\) Const: ${ReversedSequence}" $info] } {
+  puts "OK : Reverse iterator (crbegin/crend) Const"
+} else {
+  puts "Error : Reverse iterator (crbegin/crend) Const"
+}
+
+# Check previous element after full iteration, current value of Iterator is equal to last element of sequence
+if { [regexp "Current \\(last\\) element - Reverse iterator \\(crbegin/crend\\) Const: ${ReversedSequence_Last}" $info] } {
+  puts "OK : Current (last) element - Iterator (rbegin/rend) Const"
+} else {
+  puts "Error : Current (last) element - Iterator (rbegin/rend) Const"
+}