TGraph constructor enhancement

ROOT
21

I put you code in the trunk.
Thanks.

22

:wink:
Attached are equivalent ctors for TGraph2D and TGraphErrors.
Note small changes have been made to standard ctors and to the last TGraph ctor (that in the trunk) for harmonization purposes (still no changes to the corresponding header files, so the “option” name still holds).
All the test programs are included.
Cheers,
Z
tgraph_ctors.tgz (40.5 KB)

23

Thanks for these new code, but you have made a lot of cosmetics changes compare to the trunk. I would prefer you sent me your new code only without all these little changes all over the files. I need to compare what you did with the trunk and these additional changes make it hard to do.

24

[quote]… you have made a lot of cosmetics changes compare to the trunk[/quote]I’ve just used Root’s astyle.rc before posting (as recommended here: http://root.cern.ch/drupal/content/c-coding-conventions).

If you astyle-ize the corresponding trunk files, only the relevant differences will remain.
Attached are the diff files I get.
Cheers,
Z
tgraph_ctors_diff.tgz (3.18 KB)

25

I am applying astyle on these file to make the comparison simpler.
I just committed the new TGraph.cxx with astyle clean up.
Can you start from that one because I had done some mods and your new version drops them.
I will now apply astyle on TGraph2D.cxx and TGraphErrors.cxx and check your mods.

26

TGrapherrors:

I put your mods in the trunk. I have also modified a bit your code.

27

Here it is.
Cheers,
Z
TGraph.cxx (82.3 KB)

28

TGraph2D:

I put your mods in the trunk. I have also modified a bit your code.

29

TGraph:

I put your mods in the trunk. I have also modified a bit your code.

30

In you code you have :

      Char_t buffer[10000] ;

Do you really a buffer that big ?
Cannot it be dynamic ?
Then you do:

 strcpy(buffer, line.c_str())

that’s very unsafe. Your should do:

 strlcpy(buffer, line.c_str(),10000)

I’ll fix that last thing.

31

Thanks for the strlcpy hint and for the subsequent fix.

Concerning the buffer size, doing:

[code]
353c353
< TGraph::TGraph(const char *filename, const char *format, Option_t *option, int linesize)

TGraph::TGraph(const char *filename, const char *format, Option_t *option)
432c432
< Char_t * buffer = new Char_t [linesize] ;

  Char_t buffer[10000] ;

443c443
< strlcpy(buffer, line.c_str(), linesize) ; //necessary stage for strtok?

        strlcpy(buffer, line.c_str(), 10000) ; //necessary stage for strtok?

[/code]does not improve anything (same execution time, see attached test script)

Were you rather thinking about something like that: en.wikipedia.org/wiki/Strlcpy#Usage (which I haven’t tested) ?

Cheers,
Z
zzztest.cpp (1.41 KB)

32

Oops :unamused:, the comparison above is not relevant, sorry.
Please consider the archive below.
It does not change the conclusions though (similar execution time (on a 8GB RAM machine)).
Please also note, in the piece of code above, buffer should de deleted.
Cheers,
Z
tgraph_ctor4.tgz (24 KB)

33

It is a matter of code safety (reported by coverity) not performances.

I do not understand your new proposed patch … Why a new ctor ??
Ok I stick with the version we are using now. I cannot include a such fix in the trunk.

34

There is no new ctor!
The files in tgraph_ctor4.tgz (post n-1) were provided for assessment purposes only.
The corresponding changes are given in the post above (n-2) between the “code” markups.
Please find them (re-)attached in the archive below.

Note a new default argument is introduced.
So there might exist a more transparent way to proceed…
(en.wikipedia.org/wiki/Strlcpy#Usage)
What do you reckon?

Cheers,
Z
tgraph_ctor5.tgz (418 Bytes)

35

Thanks, I do not think we can change the ctor like that. It makes it confusing and not backward compatible.
Ok fine, let’s stay with the version we have now it is good enough.
I would have thought we may come with a more dynamic version.
Anyway now it is protected with stlcpy so the read cannot overwrite the char string.

36

Here’s a simpler (and a little faster) alternative (no more linesize required).
See the diff files attached for TGraph.cxx, TGraph2D.cxx and TGraphErrors.cxx.
No changes to the corresponding header files.
Cheers,
Z
tgraph_ctor6.tgz (371 Bytes)

37

There is problems using your diff files. I get:

$ patch -p0 hist/hist/src/TGraph.cxx < TGraph_cxx.diff 
patching file hist/hist/src/TGraph.cxx
Hunk #2 FAILED at 443.

Can you do the diffs against the SVN trunk using “svn diff” ?
Or send me the complete files ?

38

Here are the last 5 min trunk svn diff files.
Cheers,
Z
tgraph_ctor7.tgz (707 Bytes)

39

Your mods are now in the trunk.
Thanks.

40

Hi Olivier,

Here’s a fix to cope with DOS files (CR+LF) for non-standard ctors.
(No prob with standard ones).

Cheers,
Z
tgraph_ctor_fix.tgz (315 Bytes)