Avoid using the interpreter from TDataMember if possible. #807

Teemperor · 2017-07-25T21:21:41Z

We can't use the interpreter when generating a PCM as this would
generate AST nodes which then would end up in the module, which
is causing a long chain of modules (such as redefinitons as we
suddenly have the same cling warpper function multiple times).

In this code path we seem to always have a number that we want
to convert to a string. So let's just use atol instead here if
the argument is just a number, which should avoid the issue with
the generated code.

As we also now check if the input is a number, I added an assert
that verifies we only call atol when the string is actually a
number.

phsft-bot · 2017-07-25T21:22:13Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-07-25T21:58:17Z

Build failed on mac1012/native.
See console output.

Failing tests:

projectroot.roottest.root.html.roottest_root_html_runMakeIndex

phsft-bot · 2017-07-25T22:27:19Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

projectroot.roottest.root.html.roottest_root_html_runMakeIndex

phsft-bot · 2017-07-26T04:11:32Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-07-26T04:44:43Z

Build failed on slc6/gcc49.
See console output.

Failing tests:

projectroot.math.mathcore.test.mathcore_testLogLExecPolicy

phsft-bot · 2017-07-26T04:46:25Z

Build failed on mac1012/native.
See console output.

Failing tests:

phsft-bot · 2017-07-26T04:53:20Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

projectroot.roottest.root.html.roottest_root_html_runMakeIndex

phsft-bot · 2017-07-26T12:42:39Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-07-28T12:22:54Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

vgvassilev · 2017-08-01T06:54:37Z

@pcanal ping.

Teemperor · 2017-08-01T06:55:50Z

I have to update the PR, so: undo ping @ Philippe :)

vgvassilev · 2017-08-01T07:14:33Z

@Teemperor, did you mean updating it by reducing the scope of the fix as we discussed before? I still would like @pcanal to comment on the general direction, esp that we know we have a single use case of this code in Eve, which is the constant 9 ;)

Axel-Naumann

Tiny comments - plus you trigger the assert in roottest, see https://epsft-jenkins.cern.ch/job/root-pullrequests-build/3438/testReport/projectroot.roottest.root/html/roottest_root_html_runMakeIndex/

Axel-Naumann · 2017-08-03T15:36:03Z

core/meta/src/TDataMember.cxx

            //We'll try to find global enum existing in ROOT...
            Long_t l=0;
            Int_t  *value;
            TGlobal *enumval = gROOT->GetGlobal(ptr1,kTRUE);


Why do you call TROOT::GetGlobal() if the string is just a number?

i.e. Should the strtol be even before this call?

Axel-Naumann · 2017-08-03T15:36:37Z

core/meta/src/TDataMember.cxx

-            if (enumval){
+
+            char *strtolResult;
+            l = std::strtol(ptr1, &strtolResult, 10);


Can you move the decl of l here?

Axel-Naumann · 2017-08-03T15:37:35Z

core/meta/src/TDataMember.cxx

               if (obj)
                  l = ((TEnumConstant*)obj)->GetValue();
-               else
+               else if (!isnumber)


isnumber must be false here, right?

pcanal · 2017-08-03T19:02:47Z

So let's just use atol instead here if
the argument is just a number, which should avoid the issue with
the generated code.

Yes, checking 'early' that the string is just a number is a good idea.

phsft-bot · 2017-08-03T21:15:57Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

Teemperor · 2017-08-03T21:17:17Z

@Axel-Naumann the assert failure was for a previous version of this PR, the one you reviewed already had the fix :)

I think I addressed all comments in the newest revision, let's see if we get a green Jenkins with the current state of the master.

vgvassilev · 2017-08-03T21:25:52Z

@Teemperor I hate to say this but do you feel like adding a gtest ;) ?

Axel-Naumann · 2017-08-15T07:48:28Z

@pcanal can this be merged?

vgvassilev · 2017-08-15T10:41:08Z

@Teemperor could you fix the coding violations?

pcanal · 2017-08-15T16:58:57Z

@Axel-Naumann Yes (pending clang-formating).

phsft-bot · 2017-08-23T13:55:16Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

Teemperor · 2017-08-23T13:55:43Z

Back from vacation, formatted the code.

We can't use the interpreter when generating a PCM as this would generate AST nodes which then would end up in the module, which is causing a long chain of modules (such as redefinitons as we suddenly have the same cling warpper function multiple times). In this code path we seem to always have a number that we want to convert to a string. So let's just use strtol instead here if the argument is just a number, which should avoid the issue with the generated code. As we also now check if the input is a number, I added an assert that verifies we fail when we can't handle the given user input.

Teemperor force-pushed the AvoidInterpreterInTDataMember branch from 6f96cc3 to 930910b Compare July 26, 2017 04:11

amadio requested a review from pcanal July 26, 2017 07:59

amadio assigned pcanal Jul 26, 2017

Teemperor force-pushed the AvoidInterpreterInTDataMember branch from 930910b to 5f29f38 Compare July 26, 2017 12:42

Teemperor force-pushed the AvoidInterpreterInTDataMember branch from 5f29f38 to 7f292b5 Compare July 28, 2017 12:22

Axel-Naumann requested changes Aug 3, 2017

View reviewed changes

Teemperor force-pushed the AvoidInterpreterInTDataMember branch from 7f292b5 to 7de1718 Compare August 3, 2017 21:15

Axel-Naumann approved these changes Aug 15, 2017

View reviewed changes

Teemperor force-pushed the AvoidInterpreterInTDataMember branch from 7de1718 to 55aefa9 Compare August 23, 2017 13:55

vgvassilev merged commit 538c947 into root-project:master Aug 23, 2017

Teemperor deleted the AvoidInterpreterInTDataMember branch December 19, 2017 08:39

phsft-bot mentioned this pull request Apr 28, 2019

RDataSet experimental prototype #2778

Closed

phsft-bot mentioned this pull request Oct 23, 2020

Fix issue #6670. Avoid infinite recursion from TClass::InheritsFrom. #6680

Merged

phsft-bot mentioned this pull request Sep 14, 2022

[cxxmodules][cling] Avoid loading some unnecessary modules #10910

Closed

Avoid using the interpreter from TDataMember if possible. #807

Avoid using the interpreter from TDataMember if possible. #807

Uh oh!

Conversation

Teemperor commented Jul 25, 2017

Uh oh!

phsft-bot commented Jul 25, 2017

Uh oh!

phsft-bot commented Jul 25, 2017

Failing tests:

Uh oh!

phsft-bot commented Jul 25, 2017

Failing tests:

Uh oh!

phsft-bot commented Jul 26, 2017

Uh oh!

phsft-bot commented Jul 26, 2017

Failing tests:

Uh oh!

phsft-bot commented Jul 26, 2017

Failing tests:

Uh oh!

phsft-bot commented Jul 26, 2017

Failing tests:

Uh oh!

phsft-bot commented Jul 26, 2017

Uh oh!

phsft-bot commented Jul 28, 2017

Uh oh!

vgvassilev commented Aug 1, 2017

Uh oh!

Teemperor commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgvassilev commented Aug 1, 2017

Uh oh!

Axel-Naumann left a comment

Choose a reason for hiding this comment

Uh oh!

Axel-Naumann Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

pcanal Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

Axel-Naumann Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

Axel-Naumann Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

pcanal commented Aug 3, 2017

Uh oh!

phsft-bot commented Aug 3, 2017

Uh oh!

Teemperor commented Aug 3, 2017

Uh oh!

vgvassilev commented Aug 3, 2017

Uh oh!

Axel-Naumann commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgvassilev commented Aug 15, 2017

Uh oh!

pcanal commented Aug 15, 2017

Uh oh!

phsft-bot commented Aug 23, 2017

Uh oh!

Teemperor commented Aug 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Teemperor commented Aug 1, 2017 •

edited

Loading

Axel-Naumann commented Aug 15, 2017 •

edited

Loading