Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

COMP: Fix for gcc13.2 compiler test failures #4636

Closed
wants to merge 1 commit into from
Closed

COMP: Fix for gcc13.2 compiler test failures #4636

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmDataSet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,9 @@ class GDCM_EXPORT DataSet

// DUMB: this only search within the level of the current DataSet
bool FindDataElement(const Tag &t) const {
const DataElement r(t);
//ConstIterator it = DES.find(r);
if( DES.find(r) != DES.end() )
{
return true;
}
return false;
const auto it = GetDataElement(t);
Copy link
Member

@issakomi issakomi May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, this will trigger copy of the DataElement (the function returns const DataElement&, but this const auto = forces copy of the object, IMO, previous variant also created local DataElement, maybe
return (GetDataElement(t) != GetDEEnd());
is faster). It is not a request to change, the issue is very odd. cc @N-Dekker

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@issakomi I just had a few days off, sorry for my late response! I'm not very familiar with this code. I would expect that it would search in the DataElementSet for a DataElement with the specified tag. But instead, it locally creates a DataElement with the specified tag, using default values for its VL and VR. It then tries to find this local DataElement in the DataElementSet. So I think it can only find a DataElement with those default values for VL and VR. Is that OK?

Anyway, I agree with you that a "copy" might be skipped by doing return (GetDataElement(t) != GetDEEnd()).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect that it would search in the DataElementSet for a DataElement with the specified tag. But instead, it locally creates a DataElement with the specified tag, using default values for its VL and VR. It then tries to find this local DataElement in the DataElementSet. So I think it can only find a DataElement with those default values for VL and VR. Is that OK?

Thank you. Honestly, I don't fully understand the logic. In fact the function bool FindDataElement(const Tag &t) takes one argument, the tag. But use DataElement ==, that operator is doing much more. Not sure.

Copy link
Member

@issakomi issakomi May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done some test (Linux, GCC 12). These GetDataElement and FindDataElement rely on DES.find. DES is std::set<DataElement>. DES.find doesn't seem to use DataElement's operator==. The function works (both old and new variants) with GCC 12, but I still don't fully understand ...

Copy link
Member

@issakomi issakomi May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that comparing for find is something like !(x < y) && !(y < x)
without operator==, only with operator<. The last one uses Tag. But what was the issue with GCC 13 is still not clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@issakomi Thanks for this little research! 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that GDCM almost always does something like

  if( !subds2.FindDataElement(tps) ) return false;
  const DataElement &de = subds2.GetDataElement( tps );

FindDataElement does in fact 1st GetDataElement and 2nd compares returned one with invalid DataElement(Tag(0xffff, 0xffff)). So far 1 find too much (and it is not cheap: "Complexity: logarithmic in the size of the container")
It could be e.g. or something

  const DataElement &de = subds2.GetDataElement( tps );
  if (de == DataElement(Tag(0xffff, 0xffff)) return false;

There are dozens or hundreds of such double find s in e.g. gdcmImageHelper. Sometimes 3 times, e.g. in itkGDCMImageIO
Just FYI. I will not start PRs for this.

// Return if tag is found
return it != GetDEEnd();
}

// WARNING:
Expand Down