Issue1761139

Implementation of 'reversed(seq)' built-in function.
Attached is the zip file. Please unzip it and follow
the ReadMe.txt

This is a decent start towards reversed, but there are a several problems with the patch.

First, as a general rule, a patch should just be a single diff file.  If you svn add PySequenceReverseIter.java and test_reversed.py in your local checkout, and then run 'svn diff > reversed_builtin.diff' at the base of your checkout you'll get a single file containing all of your changes.  This is a single operation for someone else to apply it to check it out.  By zipping it up you've made it a several step operation.

Secondly, your test_reversed.py relies on someone to manually verify its output.  Any new test for Python functionality in Jython should use the unittest module and be placed in Lib/test/ so it can be run by regrtest.py.  If you do that it'll be run by regrtest and checked along with everything else.  Look at some of the files in Lib/test that use unittest for examples of how it should be done.  I'd also check for existing tests for reversed in the CPython 2.4 or 2.5 Lib/test directory.  I'd be surprised if there aren't some already and you should make sure all of those work and include them in your patch.

Now, on to the actual implementation.  I wouldn't call the class PyReversedSequenceIterator; PyReversedIterator will do since it doesn't require a Sequence.  In the same vein, you should remove the instanceof PySequence check in __builtin__. The pep just states that an object passed to reversed needs __getitem__ and __len__ methods, so just check for and use those methods.  

There's no need to check for StopIteration or catch PyException at all in your __iternext__.  You're not using the iteration functions so StopIteration has nothing to do with with your calls.

Using negative indexing probably isn't a good idea: supporting it isn't a requirement of a sequence like object.  Instead I'd get the last index with __len__, and start grabbing items with __getitem__ from it counting down.

Thanks for your comments.

I have seen sentences in the PEP like, 

"This proposal is to add a builtin function to support 
reverse iteration over sequences."

"Add a builtin function called reversed() that makes a 
reverse iterator over sequence objects that support 
__getitem__() and __len__()"

And also, in cpython implementation they verified like
in the following

+ 	if (!PySequence_Check(seq)) {

+ 		PyErr_SetString(PyExc_TypeError,

+ 		"argument to reversed() must be a sequence");

+ 		return NULL;

+ 

+ 	}

I maybe interpreted "Sequences" wrongly. 

Kindly verify once and make me understand.

http://www.python.org/doc/2.4.3/whatsnew/node7.html
http://www.python.org/dev/peps/pep-0322/

Thanks,
Mehendran

Right, that's the same thing I was reading in the pep.  It's saying it should work on anything that has __getitem__ and __len__ which doesn't correspond to PySequence in Jython.  PySequence is just the baseclass for builtin sequence types like PyList and PyTuple.  Any PyObject can implement those methods.

PySequence_Check in CPython is the following:

int
PySequence_Check(PyObject *s)
{
	if (s && PyInstance_Check(s))
		return PyObject_HasAttrString(s, "__getitem__");
	if (PyObject_IsInstance(s, (PyObject *)&PyDict_Type))
		return 0;
	return s != NULL && s->ob_type->tp_as_sequence &&
		s->ob_type->tp_as_sequence->sq_item != NULL;
}

Essentially it returns true if the object is an instance of a user-defined class with a __getitem__ attribute, or if it isn't a subclass of dict and it is a builtin sequence type.

Thanks, you clarified me. 
I will get get with modifications.

I've modified code according to cgroves. 
And I've attached a patch for that.
Kindly get back to me if there is any problem.
File Added: reversed_builtin.diff

There were still a couple problems in the latest patch, but they were small enough that I went ahead and fixed them and committed this in r3365 on the 2.3 branch.

First, don't include commented out printouts or any stuff like that that shouldn't be in the final committed code in the patch.  It should be in a state where I can apply the patch and commit it without modification.

An object being an instance of PyDictionary doesn't make it a dict in the same way that an object being a PySequence doesn't make it a sequence.  Instead of checking instanceof, I changed it to look for the keys attr like the code does in CPython.

We're trying to increase the javadoc coverage on publicly exposed methdods and classes in Jython, so anything that's at that level should have some javadocs.

I deleted the completely commented out test from test_enumerate.  Is adding a __reversed__ method to classes something from 2.6?  I didn't see anything about it in the 2.5 source I looked at.  In any case, if something is going to need additional features later, I'd recommend filing a bug for it rather than burying it in commented out source code.  No one is going to go through the source looking for things that are missing, but a bug won't get lost like that.