Fix issue ROOT-7588.

[zzxuanyuan]

cin is not seekable. Temporary stringstream is created to store cin.

This patch fixes the error. But I am afraid using intermediate stringstream is not an efficient way, especially it costs a lot of memory if the input file is large. Any suggestion? @bbockelm

[pcanal]

Is there a way to interogate the stream to see if it is seekable or not?

[zzxuanyuan]

@pcanal In the function "GetNewlineValue(inputStream)", it is trying to call tellg() and seekg() functions to read the first line of the .csv file and make sure the stream goes back to the beginning position at the end of "GetNewlineValue(inputStream)". For stringstream and fstream, I can see the position of stream "inPos" increments as the characters read in. But cin always sets "inPos" as -1.

[zzxuanyuan]

Edit: inputStream.seekg(inPos) in GetNewlineValue() function the location error happened. Since cin always set inPos as -1, this function will set FailBit in inputStream and cause the error.

[pcanal]

The question I have is whether the detection of the case where we need to use the fallback solution can be improve (comparing to std::cin address might not be sufficient (and might sometimes but a false positive).

[pcanal]

Did you get a change to analyze my question about detecting the case where we need to use a fallback?

Thanks,
Philippe.

[zzxuanyuan]

Philippe,

I am not sure if I understand "fallback" correctly. But I tried two different experiments and get more solid analysis:

1. I run "TString::ReadFile(std::istream& strm)" function in Stringio.cxx and parse a file (ifstream) as a input, it works fine. If I parse std::cin as input, it has the same problem as ROOT-7588 described. The reason is because in the function it uses "strm.tellg()" to manipulate the std::cin, which always return -1 and set failure flag.
2. I run "TString::ReadLine(std::istream& strm, Bool_t skipWhite)" and "TString::ReadString(std::istream& strm)" in Stringio.cxx. I re-run the same kind of experiments as ROOT-7588. It works fine. These functions internally call ReadToDelim() which reserves memory for TString and read std::cin into TString memory buffer. I think this implementation is a smoother way rather than creating a temporary sstream to buffer std::cin.

Anyway, I think we can either create a sstream to hold std::cin or find a new way to reserve a memory buffer for std::cin.

Does this answer the question of fallback you mentioned?

Regards,

Zhe

[pcanal]

Hi,

The patch contains

if (&inputStream == &std::cin) {

where rather than test on the address of std::cin, I wonder if there is a test we can make on the seek ability of inputStream.

Cheers,
Philippe.

[zzxuanyuan]

@pcanal I made a change. It checks if tellg() return -1 and also makes sure the stream is good.

[bbockelm]

That looks right to me.

Is there a test written for this?

[zzxuanyuan]

@pcanal I am writing roottest for this patch.

SET(THESCRIPT ${CMAKE_CURRENT_SOURCE_DIR}/parseCin.sh)
SET(THEINPUTFILE ${CMAKE_CURRENT_SOURCE_DIR}/test.csv)

ROOTTEST_GENERATE_EXECUTABLE(readFromCin readFromCin.cxx LIBRARIES Core Hist RIO Net Graf Graf3d Gpad Tree Rint Postscript Matrix Physics MathCore Thread MultiProc)

ROOTTEST_ADD_TEST(roottest-root-tree-readcin-readFromCin
                  COMMAND readFromCin test.csv
                  OUTREF test.ref
                  DEPENDS ${GENERATE_EXECUTABLE_TEST})

ROOTTEST_ADD_TEST(roottest-root-tree-readcin-parseCin
                  COMMAND ${THESCRIPT}
                  OUTREF test.ref )

ROOTTEST_ADD_TESTDIRS()

where parseCin.sh is one line bash script

cat test.csv | ./readFromCin

When I run the test case, these two command always generate empty file. test.csv never been parsed in as an argument. How should I parse a argument? I tried MACRO_ARGS but that did not work.

Thanks.

[bbockelm]

Hi Zhe,

Were you able to post the roottest branch to your github account so I could review your issue?

Brian

[zzxuanyuan]

Sure. I was compiling the roottest directly within root. I will try to reproduce my issue with standalone roottest and upload repo to my github.

Zhe

On Jan 24, 2016, at 20:08, Brian Bockelman notifications@github.com wrote:

Hi Zhe,

Were you able to post the roottest branch to your github account so I could review your issue?

Brian

—
Reply to this email directly or view it on GitHub.

[bbockelm]

To do that - simply checkout roottest into the root source directory. The build directory will utilize that instead of downloading its own copy.

[bbockelm]

@pcanal - any chance to get an official mirror of roottest? It's somewhat a PITA to review contributions otherwise...

[zzxuanyuan]

@bbockelm I accidentally deleted my previous work last night. But good news is I figured out the issue I was facing when I try to reproduce my work today.
Now everything is working. I uploaded roottest repo in my github and generated the unit test as a commit. My current test simply follows the sample code in https://sft.its.cern.ch/jira/browse/ROOT-7588.
Let me know if something is not correct.

[bbockelm]

Sounds good!

If we're happy with the approach now and have a corresponding test ... are we ready to get this merged? @pcanal - any unaddressed issues in the ticket history?

[pcanal]

it looks good. Can you remind where to pick the roottest changes?

[pcanal]

merged into the master.

Thank you very much.

[pcanal]

Could you look into this (somewhat related issues):

https://sft.its.cern.ch/jira/browse/ROOT-4877

[zzxuanyuan]

Sure I am looking at it.