Type of elements in a data set cannot be modified

[lamyj]

The type of elements in data set, once added, cannot be modified: they must be deleted and re-added. See the comments in 19cb2a8

[lamyj]

This could be solved with the following API change.

• Emptying an element:

/// Clear the contents of the element (element.get_value().get_type() will be Empty)
void Element::clear();

/// Clear the contents of an element (data_set.empty(tag) will be true)
void DataSet::clear(Tag const & tag);

• Changing the type of an element element: adapt the as_XXX setter functions in Value, Element and DataSet to return a reference to an empty array of correct type:

Value value; // empty
value.as_integers() = {1,2,3,4}; // empty -> integers
value.as_strings() = {"foo", "bar"}; // integers -> strings, integers value is lost

@cguebert: does this address your remarks?

[cguebert]

I wrote a response but I didn't read correctly the second part of your message.

Do you propose to change the type of the value depending on the accessor used?
I am not convinced it is a good idea. What happens when using the wrong getter to read a value?

Example:
dataSet[PatientName].as_real();
Will it return an empty reals array if the tag is present but the value is empty, but throw if there is a value that is a string as it should be?

I would prefer keeping with the current API, in the spirit that it prevents mistakes.

[lamyj]

Slowly getting to the end of my backlog...

We seem to have three different cases:

1. A non-empty element must be emptied (e.g. anonymization process)
2. An empty element must be filled (e.g. completion of a partial data set)
3. The type of a non-empty element must be changed (e.g. string -> float)

Case 1 is solved by the clear method from my first comment. This won't add potential mistakes, and keeps in line with the Container API.

Case 2 requires a slight modification of the non-const as_XXX: if the value is empty, then the function sets the type and returns a reference to the empty corresponding member. At that point, said member is empty (or can be cleared just to make sure). It has however a nasty side-effect in code that is not const-correct:

void f(Value & v) // f does not modify v, but v is passed by non-const reference
{
    // This calls the non-const as_int (cf. standard 13.3.3.2/3, rule beginning with 
    // "S1 and S2 are reference bindings)
    std::cout << v.as_int().size();
}

void g()
{
    Value v; // v.empty() is true
    f(v); // v now holds an empty array of integers
}

Case 3 is the one you mention. I tried to find a real-life use-case for it, but failed.

Case 1 will be implemented, case 3 won't, and, unless I missed an easy way to remove the aforementioned side-effect, case 2 won't be implemented either.

[cguebert]

I think the discussion is going way off course of the original comment. I do not want to change the type of a Value (if I wanted, I know of remove + add).

The commit 19cb2a8 changed how we access the elements in a data set.
That is, if any value is of length 0, it gets the type empty, instead of the type corresponding to the VR.
Before 19cb2a8, the code: dataSet[odil::registry::PatientName], returned a Value of type string, because PatientName is of VR PN, and PN is represented in Odil as a std::string.
Now, the code dataSet.as_string(odil::registry::PatientName) may throw even if the value exists, which is the regression I spoke of. Was your modification the only way to solve the bug in the JSON serialization ?

The safe way to access a value is now similar to this:

if ( dataSet.has(odil::registry::PatientName) && !dataSet.empty(odil::registry::PatientName) )
    myValue = dataSet.as_string(odil::registry::PatientName, 0);

I find it cumbersome and quite inefficient as it looks into the map 3 times now. (This pushed me to create a DataSet wrapper to use boost::optional in order to limit this type of access).

[lamyj]

I indeed misunderstood your initial report. The last two commits (c9cc057 and 225ddc3) restore the behavior of the as_XXX accessors to empty elements and values: as in your example, as long as PatientName was created as a string element, dataSet.as_string(odil::registry::PatientName) will now always return an array of strings.

For more technical details, the commits remove the Empty type in Value and Element. The prototype of DataSet is unchanged, but Value and Element are modified:

• Value is no more default-constructible
• Element is no more default-constructible, but gains a constructor creating an empty element with the type deduced from the VR

Tell me if this solves the issue.