Re: [Gdcm2] gdcm::CodeString and bug 2901256 [Was: Merging trunk to gdcm-2-0]

[Mathieu M. <mat...@gm...>]

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