Thread: [Gdcm2] Merging trunk to gdcm-2-0

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Just a quick note, gdcm-2-0 is being updated from trunk. diffstat is currently:

http://gdcm.sourceforge.net/thingies/gdcm-2-0-diffstat.txt

Stay tuned for the next release...
-- 
Mathieu

Hi Mathieu  :-)

You wrote:
> Just a quick note, gdcm-2-0 is being updated from trunk. diffstat is
> currently:
> http://gdcm.sourceforge.net/thingies/gdcm-2-0-diffstat.txt
> Stay tuned for the next release...

Cool...  I guess branches/gdcm-2-0 now basically has a preliminary version 
of the upcoming release, right?
(I mean: <https://gdcm.svn.sourceforge.net/svnroot/gdcm/branches/gdcm-2-0>)

Note: When I retrieved all files from branches/gdcm-2-0 and did a default 
Configure + Generate, it said:

  CMake Error in Testing/Source/Common/Cxx/CMakeLists.txt:
  Cannot find source file "TestCommand".  Tried extensions .c .C .c++ .cc
  .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp .hxx .in .txx

I don't have this problem when I get the latest version from the trunk. My 
default generator is Visual Studio 9 2008; I tried both CMake 2.8.0 and 
2.8.1. Do you know what this CMake error is about?

Kind regards, Niels
--
Niels Dekker
http://www.xs4all.nl/~nd/dekkerware
Scientific programmer at LKEB, Leiden University Medical Center 

Hi Niels,

On Thu, Jun 24, 2010 at 6:56 PM, Niels Dekker - address until
2010-10-10 <nie...@xs...> wrote:
> Hi Mathieu  :-)
>
> You wrote:
>> Just a quick note, gdcm-2-0 is being updated from trunk. diffstat is
>> currently:
>> http://gdcm.sourceforge.net/thingies/gdcm-2-0-diffstat.txt
>> Stay tuned for the next release...
>
> Cool...  I guess branches/gdcm-2-0 now basically has a preliminary version
> of the upcoming release, right?
> (I mean: <https://gdcm.svn.sourceforge.net/svnroot/gdcm/branches/gdcm-2-0>)
>
> Note: When I retrieved all files from branches/gdcm-2-0 and did a default
> Configure + Generate, it said:
>
>  CMake Error in Testing/Source/Common/Cxx/CMakeLists.txt:
>  Cannot find source file "TestCommand".  Tried extensions .c .C .c++ .cc
>  .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp .hxx .in .txx
>
> I don't have this problem when I get the latest version from the trunk. My
> default generator is Visual Studio 9 2008; I tried both CMake 2.8.0 and
> 2.8.1. Do you know what this CMake error is about?

Sorry my bad. I am not familiar with how svn merge works, but for some
reason all added files from trunk when merge to gdcm-2-0 branch were
discarded. I repaired that:

$ svn ci -m"Forgot newly added files during the svn merge... thanks to
Niels for report."
Adding         Testing/Source/Common/Cxx/TestCommand.cxx
Adding         Testing/Source/Common/Cxx/TestCryptographicMessageSyntax.cxx
Adding         Testing/Source/DataStructureAndEncodingDefinition/Cxx/TestPDBHeader.cxx
Adding         Testing/Source/MediaStorageAndFileFormat/Cxx/TestAnonymizeEvent.cxx
Adding         Testing/Source/MediaStorageAndFileFormat/Cxx/TestDICOMDIRGenerator.cxx
Adding         Testing/Source/MediaStorageAndFileFormat/Cxx/TestFileDerivation.cxx
Adding         Testing/Source/MediaStorageAndFileFormat/Cxx/TestImageChangeTransferSyntax1.cxx
Adding         Testing/Source/MediaStorageAndFileFormat/Cxx/TestImageReaderRandomEmpty.cxx
Adding         Testing/Source/MediaStorageAndFileFormat/Cxx/TestPNMCodec.cxx
Adding         Testing/Source/MediaStorageAndFileFormat/Cxx/TestSegmentedPaletteColorLookupTable.cxx
Adding         Testing/Source/MediaStorageAndFileFormat/Cxx/TestSerieHelper.cxx
Adding         Testing/Source/MediaStorageAndFileFormat/Cxx/TestSplitMosaicFilter.cxx
Transmitting file data ............
Committed revision 6768.

Thanks !
-- 
Mathieu

Mathieu Malaterre wrote:
> $ svn ci -m"Forgot newly added files during the svn merge...
> thanks to Niels for report."

You're welcome  :-)  Now the CMake error is gone, indeed  :-)

Is branches/gdcm-2-0 nog basically a copy of gdcm/trunk?  Or will there be 
any essential difference between the trunk and the upcoming release?

Note: a default generated MSVC 2008 build now compiles fine. However, when I 
switch on GDCM_BUILD_SHARED_LIBS, I will still get link error (LNK2005), 
beacuse of bug 2901256, "CodeString related link error (LNK2005) in 
gdcmDSED", 
http://sourceforge.net/tracker/?func=detail&aid=2901256&group_id=137895&atid=739587 
I'm sorry I haven't fixed it yet, and I'm pretty busy right now. Xiaofeng 
Zhao already sent me a proposed patch, a few months ago.

If you want to go for a quickfix, just remove the GDCM_EXPORT from the 
CodeString class, and make CodeString::IsValid() inline, inside 
gdcmCodeString.h

FWIW, I usually don't switch on GDCM_BUILD_SHARED_LIBS  :-)

HTH,

  Niels
--
Niels Dekker
http://www.xs4all.nl/~nd/dekkerware
Scientific programmer at LKEB, Leiden University Medical Center

On Fri, Jun 25, 2010 at 11:06 AM, Niels Dekker - address until
2010-10-10 <nie...@xs...> wrote:
> Is branches/gdcm-2-0 nog basically a copy of gdcm/trunk?  Or will there be
> any essential difference between the trunk and the upcoming release?

It is basically a copy of trunk. There is no difference from the trunk
and the release ATM.

> Note: a default generated MSVC 2008 build now compiles fine. However, when I
> switch on GDCM_BUILD_SHARED_LIBS, I will still get link error (LNK2005),
> beacuse of bug 2901256, "CodeString related link error (LNK2005) in
> gdcmDSED",
> http://sourceforge.net/tracker/?func=detail&aid=2901256&group_id=137895&atid=739587
> I'm sorry I haven't fixed it yet, and I'm pretty busy right now. Xiaofeng
> Zhao already sent me a proposed patch, a few months ago.
>
> If you want to go for a quickfix, just remove the GDCM_EXPORT from the
> CodeString class, and make CodeString::IsValid() inline, inside
> gdcmCodeString.h
>
> FWIW, I usually don't switch on GDCM_BUILD_SHARED_LIBS  :-)

I have applied a patch to trunk, so that CodeString now has a member
variable that is a gdcm::String. See attach file for full patch.
Let me know what you think of this patch.

Thanks,
-- 
Mathieu

>> http://sourceforge.net/tracker/?func=detail&aid=2901256&group_id=137895&atid=739587
>> I'm sorry I haven't fixed it yet, and I'm pretty busy right now.

Mathieu Malaterre wrote:
> I have applied a patch to trunk, so that CodeString now has a member
> variable that is a gdcm::String. See attach file for full patch.
> Let me know what you think of this patch.

I'm sorry, I just don't know! First of all I don't use gdcm::CodeString 
myself. And I'm not sure if I fully understand what it is about. It looks to 
me like it has just a single relevant member function: 
CodeString::IsValid(). If so, could the whole class possibly be replaced by 
a single stand-alone function, 
gdcm::IsValidCodeString(gdcm::String<'\\',16>)? Or a gdcm::String member 
function, gdcm::String::IsValidCodeString(void)?

So at the moment I just don't know how the design of gdcm::CodeString could 
possibly be improved, because I don't understand its use cases well enough. 
All I know for sure now is that it would be nice to have those those link 
errors fixed  :-)

Kind regards, Niels
--
Niels Dekker
http://www.xs4all.nl/~nd/dekkerware
Scientific programmer at LKEB, Leiden University Medical Center 

> Mathieu Malaterre wrote:
>> I have applied a patch to trunk, so that CodeString now has a member
>> variable that is a gdcm::String. See attach file for full patch.
>> Let me know what you think of this patch.
>
> I'm sorry, I just don't know! First of all I don't use gdcm::CodeString
> myself. And I'm not sure if I fully understand what it is about. It looks to
> me like it has just a single relevant member function:
> CodeString::IsValid(). If so, could the whole class possibly be replaced by
> a single stand-alone function,
> gdcm::IsValidCodeString(gdcm::String<'\\',16>)? Or a gdcm::String member
> function, gdcm::String::IsValidCodeString(void)?

FWIW, Yesterday I hadn't yet realized that gdcm::CodeString is an 
implementation of the DICOM CS value representation. So now I think I 
understand better why you decided to have it as a separate class, 
instead of just using gdcm::String.  And I think your modification of 
yesterday does indeed improve the design of gdcm::CodeString  :-)

Personally I'd rather have the validity check inside the constructors of 
gdcm::CodeString, in order to avoid the creation of invalid code 
strings. But as long as people have the discipline of always calling 
IsValid() after constructing a CodeString, I guess it should work.

BTW, PS 3.5-2008 Table 6.2-1 defines Code String (CS) as "A string of 
characters with leading or trailing spaces (20H) being non-significant." 
  I'm not sure about the meaning of "non-significant". When you do:

   std::cout << CodeString("   x"); // leading spaces

Should it have the same output as the following?

   std::cout  << CodeString("x   "); // trailing spaces

If so, it might be useful to do a Trim() inside the implementation of 
operator<< (in gdcmCodeString.h), or even inside the CodeString 
constructors.

Note that if you'd do the Trim() inside the the CodeString constructors, 
it would make the String<'\\',16>::IsValid() check more tolerant towards 
lengthy strings that have surrounding spaces.

HTH,

   Niels

-- 
Niels Dekker
http://www.xs4all.nl/~nd/dekkerware
Scientific programmer at LKEB, Leiden University Medical Center

On Sat, Jun 26, 2010 at 4:14 PM, Niels Dekker - address until
2010-10-10 <nie...@xs...> wrote:
>> Mathieu Malaterre wrote:
>>> I have applied a patch to trunk, so that CodeString now has a member
>>> variable that is a gdcm::String. See attach file for full patch.
>>> Let me know what you think of this patch.
>>
>> I'm sorry, I just don't know! First of all I don't use gdcm::CodeString
>> myself. And I'm not sure if I fully understand what it is about. It looks to
>> me like it has just a single relevant member function:
>> CodeString::IsValid(). If so, could the whole class possibly be replaced by
>> a single stand-alone function,
>> gdcm::IsValidCodeString(gdcm::String<'\\',16>)? Or a gdcm::String member
>> function, gdcm::String::IsValidCodeString(void)?
>
> FWIW, Yesterday I hadn't yet realized that gdcm::CodeString is an
> implementation of the DICOM CS value representation. So now I think I
> understand better why you decided to have it as a separate class,
> instead of just using gdcm::String.  And I think your modification of
> yesterday does indeed improve the design of gdcm::CodeString  :-)
>
> Personally I'd rather have the validity check inside the constructors of
> gdcm::CodeString, in order to avoid the creation of invalid code
> strings. But as long as people have the discipline of always calling
> IsValid() after constructing a CodeString, I guess it should work.
>
> BTW, PS 3.5-2008 Table 6.2-1 defines Code String (CS) as "A string of
> characters with leading or trailing spaces (20H) being non-significant."
>  I'm not sure about the meaning of "non-significant". When you do:
>
>   std::cout << CodeString("   x"); // leading spaces
>
> Should it have the same output as the following?
>
>   std::cout  << CodeString("x   "); // trailing spaces
>
> If so, it might be useful to do a Trim() inside the implementation of
> operator<< (in gdcmCodeString.h), or even inside the CodeString
> constructors.
>
> Note that if you'd do the Trim() inside the the CodeString constructors,
> it would make the String<'\\',16>::IsValid() check more tolerant towards
> lengthy strings that have surrounding spaces.

Point taken. I made some change according to your suggestion. I have
explained in the documentation the multiple reason why I cannot do the
validation on the fly at cstion time.
Thanks for your inputs !

$ svn ci -m"Cleanup CodeString interface. Add doc. Make sure to Trim
at cstion time. Explain with validation is not perform in cstor. add
test to check comparion of leading/ending codestring is working."
Sending        Source/Common/gdcmString.h
Sending        Source/DataStructureAndEncodingDefinition/gdcmCodeString.cxx
Sending        Source/DataStructureAndEncodingDefinition/gdcmCodeString.h
Sending        Source/MediaStorageAndFileFormat/gdcmDICOMDIRGenerator.cxx
Sending        Testing/Source/DataStructureAndEncodingDefinition/Cxx/TestCodeString.cxx
Transmitting file data .....
Committed revision 6773.

regards,
-- 
Mathieu

>> Note that if you'd do the Trim() inside the the CodeString
>> constructors, it would make the String<'\\',16>::IsValid() check
>> more tolerant towards lengthy strings that have surrounding spaces.

Mathieu Malaterre wrote:
> Point taken.

Actually I have no idea whether it is officially according to the DICOM 
standard to accept a code string whose leading or trailing spaces cause it 
to exceed the maximun of 16 characters. I'm no DICOM guru  :-) But it 
probably wouldn't harm to be a little bit tolerant in this case.

> I made some change according to your suggestion. I have
> explained in the documentation the multiple reason why I cannot do the
> validation on the fly at cstion time.

> Thanks for your inputs !

You're welcome!

> $ svn ci -m"Cleanup CodeString interface. Add doc. Make sure to Trim
> at cstion time. Explain with validation is not perform in cstor. add
> test to check comparion of leading/ending codestring is working."
> Sending        Source/Common/gdcmString.h
> Sending
> Source/DataStructureAndEncodingDefinition/gdcmCodeString.cxx
> Sending
> Source/DataStructureAndEncodingDefinition/gdcmCodeString.h
> Sending
> Source/MediaStorageAndFileFormat/gdcmDICOMDIRGenerator.cxx
> Sending
> Testing/Source/DataStructureAndEncodingDefinition/Cxx/TestCodeString.cxx
> Transmitting file data .....
> Committed revision 6773.

Please note: Now it still has link errors, when people select 
"GDCM_BUILD_SHARED_LIBS". The CodeString class needs to have GDCM_EXPORT, in 
order to link the CodeString::IsValid() implementation from its CXX file.

HTH,

  Niels
--
Niels Dekker
http://www.xs4all.nl/~nd/dekkerware
Scientific programmer at LKEB, Leiden University Medical Center 

On Mon, Jun 28, 2010 at 12:07 PM, Niels Dekker - address until
2010-10-10 <nie...@xs...> wrote:
> Please note: Now it still has link errors, when people select
> "GDCM_BUILD_SHARED_LIBS". The CodeString class needs to have GDCM_EXPORT, in
> order to link the CodeString::IsValid() implementation from its CXX file.

This has been fixed.

Thx
-- 
Mathieu

>> Please note: Now it still has link errors, when people select
>> "GDCM_BUILD_SHARED_LIBS". The CodeString class needs to have GDCM_EXPORT, in
>> order to link the CodeString::IsValid() implementation from its CXX file.

Mathieu Malaterre wrote:
> This has been fixed.
>
> Thx

Cool! I hope it can still be included with GDCM release 2.0.15.

Some more CodeString remarks:

- CodeString::Superclass is no longer the superclass (base class) of 
CodeString. So maybe it should be renamed, e.g., InternalType.

- CodeString's operator== and operator!= are member functions. However, 
C++ guru's ;-) prefer those two operators to be declared as non-member 
functions. Especially because member functions are a-symmetrical with 
respect to their arguments.  While you'd want the compiler to treat lhs 
and rhs in an equal way, e.g., w.r.t. implicit conversion. See also 
http://stackoverflow.com/questions/1905439/overload-operators-as-member-operator-or-non-member-operator

HTH,

   Niels
-- 
Niels Dekker
http://www.xs4all.nl/~nd/dekkerware
Scientific programmer at LKEB, Leiden University Medical Center

> Some more CodeString remarks:
>
> - CodeString::Superclass is no longer the superclass (base class) of
> CodeString. So maybe it should be renamed, e.g., InternalType.
>
> - CodeString's operator== and operator!= are member functions.
> However, C++ guru's ;-) prefer those two operators to be declared as
> non-member functions.

Here's an example to illustrate this remark. Suppose we use our own 
ModalityCodeString class, here in Leiden. In order to interact with GDCM, it 
has a conversion operator from ModalityCodeString to gdcm::CodeString:

  class ModalityCodeString
  {
    char m_codeString[10];
  public:
    operator gdcm::CodeString() const
    {
      return gdcm::CodeString(m_codeString);
    }
    ...
  };

Suppose ModalityCodeString is optimized for modality code strings, for 
example by having only 10 bytes of data. (Which seems sufficient, because a 
modality code string never has more than 8 characters, as far as I know.) 
Still automatic conversion to gdcm::CodeString seems useful, for example in 
order to compare their values:

  ModalityCodeString modalityCodeString = GetModalityCodeString();
  gdcm::CodeString gdcmCodeString = GetGdcmCodeString();

  if ( modalityCodeString == gdcmCodeString )
    ...

However, this won't compile, because the operator== of gdcm::CodeString is a 
member function. If there would be a non-member, operator==(const 
gdcm::CodeString&, const gdcm::CodeString&), the comparison between 
ModalityCodeString and gdcm::CodeString would work fine, even when the 
ModalityCodeString is at the left hand side.

My 2 cents,

  Niels

--
Niels Dekker
http://www.xs4all.nl/~nd/dekkerware
Scientific programmer at LKEB, Leiden University Medical Center

Niels,

On Thu, Jul 1, 2010 at 7:50 PM, Niels Dekker - address until
2010-10-10 <nie...@xs...> wrote:
>> Some more CodeString remarks:
>>
>> - CodeString::Superclass is no longer the superclass (base class) of
>> CodeString. So maybe it should be renamed, e.g., InternalType.
>>
>> - CodeString's operator== and operator!= are member functions.
>> However, C++ guru's ;-) prefer those two operators to be declared as
>> non-member functions.
>
> Here's an example to illustrate this remark. Suppose we use our own
> ModalityCodeString class, here in Leiden. In order to interact with GDCM, it
> has a conversion operator from ModalityCodeString to gdcm::CodeString:
>
>  class ModalityCodeString
>  {
>    char m_codeString[10];
>  public:
>    operator gdcm::CodeString() const
>    {
>      return gdcm::CodeString(m_codeString);
>    }
>    ...
>  };
>
> Suppose ModalityCodeString is optimized for modality code strings, for
> example by having only 10 bytes of data. (Which seems sufficient, because a
> modality code string never has more than 8 characters, as far as I know.)
> Still automatic conversion to gdcm::CodeString seems useful, for example in
> order to compare their values:
>
>  ModalityCodeString modalityCodeString = GetModalityCodeString();
>  gdcm::CodeString gdcmCodeString = GetGdcmCodeString();
>
>  if ( modalityCodeString == gdcmCodeString )
>    ...
>
> However, this won't compile, because the operator== of gdcm::CodeString is a
> member function. If there would be a non-member, operator==(const
> gdcm::CodeString&, const gdcm::CodeString&), the comparison between
> ModalityCodeString and gdcm::CodeString would work fine, even when the
> ModalityCodeString is at the left hand side.

My bad ! I completely forgot about your proposed patch. This should be
fixed now:

$ svn ci -m"Forgot to apply proposed patch from Niels...my bad"
Sending        trunk/Source/DataStructureAndEncodingDefinition/gdcmCodeString.h
Sending        trunk/Testing/Source/DataStructureAndEncodingDefinition/Cxx/TestCodeString.cxx
Transmitting file data ..
Committed revision 6805.

Thanks for the reminder !
-- 
Mathieu

Mathieu Malaterre wrote:
> $ svn ci -m"Forgot to apply proposed patch from Niels...my bad"
> Sending
> trunk/Source/DataStructureAndEncodingDefinition/gdcmCodeString.h
> Sending
> trunk/Testing/Source/DataStructureAndEncodingDefinition/Cxx/TestCodeString.cxx
> Transmitting file data ..
> Committed revision 6805.

Thanks!  BTW, Now that all of the constructors of gdcm::CodeString already 
trim the internal string, there's no need for gdcm::CodeString::Trim() to do 
so anymore! gdcm::CodeString::Trim() now does:

  std::string Trim() const {
    return Internal.Trim();
  }

But this->Internal has been trimmed already, beforehand  :-D

Anyway, that's just a minor performance issue... I think gdcm::CodeString is 
now ready for the next release.

Kind regards, Niels

--
Niels Dekker
http://www.xs4all.nl/~nd/dekkerware
Scientific programmer at LKEB, Leiden University Medical Center

Thread: [Gdcm2] Merging trunk to gdcm-2-0

Cross-platform DICOM implementation

gdcm-developers