Fix shadowing proper parameter values by the xml comments elements

[dudarboh]

BEGINRELEASENOTES

• Fix #43, a bug where xml comment fields were processed as a legitimate values and shadowed any data that comes after the comment.

ENDRELEASENOTES

Tests

---

steer.xml

    <parameter name="LCIOInputFiles">
            <!-- test_comment -->
            data_test1.slcio
            data_test2.slcio
            data_test3.slcio
        </parameter>

Before PR:

 ***********************************************
 A runtime error occured - (uncaught exception):
      /cvmfs/ilc.desy.de/sw/x86_64_gcc82_centos7/v02-02-03/lcio/v02-17/src/cpp/src/MT/LCReader.cc (l.49) in open: Couldn't open input stream 'test_comment' [not_open]
 Marlin will have to be terminated, sorry.
 ***********************************************

After PR:

LCIO Input Files:
data_test1.slcio
data_test2.slcio
data_test3.slcio

---

steer.xml

    <parameter name="LCIOInputFiles">
            data_test1.slcio
            data_test2.slcio
            <!-- test_comment -->
            data_test3.slcio
        </parameter>

Before PR:

LCIO Input Files:
data_test1.slcio
data_test2.slcio

After PR:

LCIO Input Files:
data_test1.slcio
data_test2.slcio
data_test3.slcio

---

Simple tests that don't change the behaviour

<parameter name="LCIOInputFiles" value="data_test1.slcio data_test2.slcio"/>
<parameter name="LCIOInputFiles" value="data_test1.slcio data_test2.slcio"></parameter>
<parameter name="LCIOInputFiles">data_test1.slcio data_test2.slcio</parameter>
<parameter name="LCIOInputFiles" default="filename.slcio" type="string"> data_test1.slcio data_test2.slcio</parameter>

Before/After PR:

. . .
LCIO Input Files:
data_test1.slcio
data_test2.slcio
. . .

---

Let me know if I should check other variations.

[dudarboh]

I would add some debug printouts, but I was hesitant to use std::cout...

[tmadlener]

Hi @dudarboh,

Thanks for actually diving into this and fixing the underlying issue. Looks like you found a quite elegant fix in the end. I am currently having a look to see if there is an easy way of adding some tests for this. I will do that once I found it.

~Can you rebase this onto the latest master to pick up a working CI configuration for now, please?~

[dudarboh]

Great, thanks for the test processor!

Do you think it makes sense to add some tests, where parameters are added with a value keyword?

As it enters completely different if branch and has a completely different readout logic: (which probably can be removed(?)/refactored better, but I didn't try) https://github.com/iLCSoft/Marlin/blob/f56c872894f9b5575db507fb34babf7b57846200/source/src/XMLParser.cc#L384-L387

Comments there look very weird to even add them in the first place, but it still should read parameters properly w/o any comments.

[tmadlener]

How would a comment look like in the value use-case in xml? In any case adding more ways to parse parameters to the existing tests should be fairly straight forward now.

[tmadlener]

It looks like trying to put a comment into an explicit value tag actually breaks parsing, so that should not be a problem at all. I have added one simple test case that checks if things work also for that. Additionally, the tests are now actually checking the expected content, not just the expected number of parameters.