Invalid item in loop over sequences

[nbeck]

Hello!

i've stumbled across some unexpected error messages today, when performing loops over sequences of the following kind:

Code: Select all

	do
	{
		DRTROIContourSequence::Item& currentContour = rtROIContourSequence.getCurrentItem();
		if (currentContour.isValid())
		{
			// doSomething
		}
		else {
			std::cerr << "Error: Invalid item "<< std::endl;
		}
	} while (rtROIContourSequence.gotoNextItem().good());

The loop always yields one invalid item before exiting.

It appears that there is a problem in the gotoNextItem() method.

Code: Select all

OFCondition DRTROIContourSequence::gotoNextItem()
{
    OFCondition result = EC_IllegalCall;
    if (CurrentItem != SequenceOfItems.end())
    {
        ++CurrentItem;
        result = EC_Normal;
    }
    return result;
}

I believe, the iterator should be incremented before comparing against SequenceOfItems.end(). Any thoughts?

Note that I am using the standard library instead of the built-in types of DCMTK.

[J. Riesmeier]

Thank you for the report. We have to check this and will get back to you then.

[J. Riesmeier]

Could you please check whether the following solves the problem?

Code: Select all

OFCondition DRTROIContourSequence::gotoNextItem()
{
    OFCondition result = EC_IllegalCall;
    if (++CurrentItem != SequenceOfItems.end())
    {
        if (*CurrentItem != NULL)
            result = EC_Normal;
        else
            result = EC_CorruptedData;
    }
    return result;
}

[nbeck]

Thank you for the suggestion. With this code everything works as expected! It also matches the implementation in other parts of DCMTK, e.g. DSRCodingSchemeIdentificationList::gotoNextItem() or DSRReferencedInstanceList::gotoNextItem(), so it is hopefully well tested.

You are probably aware of this, but just to avoid any confusion: This issue isn't specific to DRTROIContourSequence but affects most (or all) of the sequences in DCMRT.

In my humble opinion, the code would be much easier to maintain if these sequences were derived from a common templated class instead of duplicating the list code for each one. I tried to work on a patch for this, but since there are so many affected files which were generated by some script, I quickly gave up.

[J. Riesmeier]

Thank you for the suggestion. With this code everything works as expected! It also matches the implementation in other parts of DCMTK, e.g. DSRCodingSchemeIdentificationList::gotoNextItem() or DSRReferencedInstanceList::gotoNextItem(), so it is hopefully well tested.

Thank you for your feedback. This is also what I was thinking. Don't ask me why the dcmrt implementation is different. At least the author was the same

You are probably aware of this, but just to avoid any confusion: This issue isn't specific to DRTROIContourSequence but affects most (or all) of the sequences in DCMRT.

You are right and, of course, I am also aware of this.

In my humble opinion, the code would be much easier to maintain if these sequences were derived from a common templated class instead of duplicating the list code for each one. I tried to work on a patch for this, but since there are so many affected files which were generated by some script, I quickly gave up.

That's true, but more than 90% of the dcmrt code (including the one of the sequences classes) is generated automatically from a machine-readable version of the DICOM standard, so it makes no difference. The dcmrt module was the result of a common research project with the German Cancer Research Center (DKFZ, Heidelberg).

[nbeck]

I understand. Thank you for your efforts, I am looking forward to the next release.

[J. Riesmeier]

Thank you for your feedback. By the way, I also plan to update the automatically generated dcmrt classes to the latest edition of the DICOM standard, i.e. DICOM 2023a. I already started with this work, but, as always, there are still some issues: the DICOM RT people like to use the same Sequence Attributes with different content in either different Modules or even within the same Module!

[J. Riesmeier]

The reported issue has been fixed in the current development version (see git repository). I've also updated the automatically generated classed to DICOM 2023b.