Simplify implementation of TClass::FindClassOrBaseMethodWithId() #908

amadio · 2017-08-30T11:30:42Z

Test PR, do not merge please.

phsft-bot · 2017-08-30T11:30:53Z

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

amadio · 2017-08-30T13:54:02Z

@pcanal This isn't really work in progress, I just wanted to test that the build would pass. Actually, if you think this commit is ok, we can merge this now, but please let me merge it myself locally, otherwise the GPG signature on the commit is lost.

pcanal · 2017-08-30T14:27:59Z

but please let me merge it myself locally, otherwise the GPG signature on the commit is lost.

What do you mean? How is that different from other PR?

if you think this commit is ok

I am 'slightly' surprised it works in C++11 ...

amadio · 2017-08-30T14:33:36Z

The GitHub interface strips GPG signatures from the commits. I usually merge on my laptop and push, and that way it's as if I pressed merge, but I can keep the GPG signatures.

As for the commit, if you look at the date, you'll see it's from a while ago, when I was working on I/O optimizations. I was just trying to improve readability and test range for with ROOT containers. Yes, it works :-) So, I guess we should be trying to use this more than the usual while loop. What do you think?

pcanal · 2017-08-30T14:43:01Z

but I can keep the GPG signatures.

What is the advantage/gain/importance of doing so?

pcanal · 2017-08-30T14:53:23Z

Yes, it works :-)

Indeed, I was confusing with the C++17 features

if (init; condition) and switch (init; condition)

like

if (const auto [it, inserted] = map.insert( {"foo", bar} ); inserted)

pcanal · 2017-08-30T14:57:34Z

core/meta/src/TClass.cxx

-   TFunction *f = GetMethodList()->Get(declId);
-   if (f) return (TMethod*)f;
+   if (auto method = GetMethodList()->Get(declId))
+      return reinterpret_cast<TMethod *>(method);


'Thanks' to the auto keyword the validity of the reinterpret_cast becomes harder to Check. (i.e. now one that to find out that GetMethodList returns a TListOfFunctions and then look up the return type of Get ... only then can one know that the cast is okay since TMethod inherits directly for the return type, TFunction).

Well, it must be fine, since that's what the function returns. Using reinterpret_cast is better than just a C cast, as it will fail in more cases where the C cast would compile and give wrong results.

Well, it must be fine,

It is fine of course. My point is about code readability (i.e. the ability for the human reader to verify the code :) ).

I.e. in this case, because of the reinterpret_cast, I would use

if (TFunction *method = GetMethodList()->Get(declId)) return reinterpret_cast<TMethod *>(method);

amadio · 2017-08-30T15:02:12Z

The advantage in keeping GPG signatures is that it tells people that the change came from a trusted source. We should all be doing this, but due to being complicated, I understand that others don't bother.

pcanal · 2017-08-30T15:02:28Z

core/meta/src/TClass.cxx

-      }
-   }
-   return 0;
+   for (auto item : *GetListOfBases())


One hesitation I have with this pattern is that the iterator cast is moved one level down. i.e.

while ((item = reinterpret_cast<TBaseClass *>(next()))) { if (auto base = item->GetClassPointer())

vs

for (auto item : *GetListOfBases()) if (auto base = reinterpret_cast<TBaseClass *>(item)->GetClassPointer())

or more often.

for (auto item : *GetListOfBases()) { auto base = reinterpret_cast<TBaseClass *>(item); if (base->GetClassPointer())

I wonder if we can have something like

for (auto item : ContaineeIs<TBaseClass*>(GetListOfBases())) if (auto base = item->GetClassPointer())

Let's not sidestep a 5 line commit into a lenghty discussion on code patterns. If you disagree that the code is more readable after the changes, just let me know and I'll close the PR.

pcanal · 2017-08-30T15:15:05Z

Let's not sidestep a 5 line commit into a lenghty discussion on code patterns.

My points were triggered by your good point:

So, I guess we should be trying to use this more than the usual while loop.

where I think to be really recommended as the new pattern we should slightly improve on it.

Could you, as a subsequent PR, try to improve the collection in the direction I mentioned (assuming you agree it is 'even' better :)

Anyway

If you disagree that the code is more readable

I agree it is more readable except for the auto method which should go back to TFunction *method.

Thanks,
Philippe.

amadio · 2017-08-30T15:16:12Z

Ok, I will use TFunction instead of auto, then I can merge, then?

pcanal · 2017-08-30T15:16:32Z

Ok, I will use TFunction instead of auto, then I can merge, then?

Yes. Thanks.

amadio · 2017-08-30T15:16:46Z

By improve the collection you mean changing all while loops to range fors?

pcanal · 2017-08-30T15:19:03Z

By improve the collection you mean changing all while loops to range fors?

Not quite (as this might be a good thing for the code refactoring tool to do).

I mean to enable something like:

for (auto item : ContaineeIs<TBaseClass*>(GetListOfBases()))
   if (auto base = item->GetClassPointer())

amadio · 2017-08-30T15:23:36Z

Sure, I don't know how complicated that may be, but if it's doable, we can try in a later PR. Thanks.

phsft-bot · 2017-08-30T15:28:38Z

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

pcanal · 2017-08-30T15:34:39Z

core/meta/src/TClass.cxx

-   return 0;
+   for (auto item : *GetListOfBases())
+      if (auto base = reinterpret_cast<TBaseClass *>(item)->GetClassPointer())
+         if (auto method = base->FindClassOrBaseMethodWithId(declId))


one more TFunction*. Sorry to be a pest :)

Ok, will do, no problem.

phsft-bot · 2017-08-30T15:36: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

phsft-bot · 2017-08-30T15:42:21Z

Build failed on slc6/gcc62.
See console output.

phsft-bot · 2017-08-30T16:11:41Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

projectroot.roottest.python.pythonizations.roottest_python_pythonizations_smartptr

phsft-bot · 2017-08-30T16:15:56Z

Build failed on mac1012/native.
See console output.

Failing tests:

projectroot.roottest.python.pythonizations.roottest_python_pythonizations_smartptr

phsft-bot · 2017-08-30T16:34:10Z

Build failed on centos7/gcc49.
See console output.

Failing tests:

projectroot.roottest.python.pythonizations.roottest_python_pythonizations_smartptr

phsft-bot · 2017-08-30T16:58:18Z

Build failed on slc6/gcc49.
See console output.

Errors:

ERROR: Timeout after 10 minutes
ERROR: Error fetching remote repo 'origin'
stderr: error: RPC failed; result=18, HTTP code = 200
ERROR: Error fetching remote repo 'origin'

Failing tests:

projectroot.roottest.python.pythonizations.roottest_python_pythonizations_smartptr

"too" is too general and may also match the build path. I could not find "too slow" being printed anywhere, so just check the output for "too inaccurate".

amadio requested a review from Axel-Naumann as a code owner August 30, 2017 11:30

amadio added the do not merge label Aug 30, 2017

pcanal changed the title ~~Simplify implementation of TClass::FindClassOrBaseMethodWithId()~~ [WIP] Simplify implementation of TClass::FindClassOrBaseMethodWithId() Aug 30, 2017

amadio removed the do not merge label Aug 30, 2017

amadio changed the title ~~[WIP] Simplify implementation of TClass::FindClassOrBaseMethodWithId()~~ Simplify implementation of TClass::FindClassOrBaseMethodWithId() Aug 30, 2017

pcanal reviewed Aug 30, 2017

View reviewed changes

amadio force-pushed the root-io branch from cca5fe5 to 67fe183 Compare August 30, 2017 15:28

pcanal reviewed Aug 30, 2017

View reviewed changes

Simplify implementation of TClass::FindClassOrBaseMethodWithId()

b95526d

amadio force-pushed the root-io branch from 67fe183 to b95526d Compare August 30, 2017 15:36

amadio merged commit b95526d into root-project:master Aug 30, 2017

amadio deleted the root-io branch August 30, 2017 15:38

phsft-bot mentioned this pull request Feb 17, 2022

[RF] Cleanup RooLagrangianMorphFunc #9912

Merged

2 tasks

phsft-bot mentioned this pull request Apr 20, 2022

[RF] Remove deprecated version of createHistogram to see what happens #10419

Closed

phsft-bot mentioned this pull request Jun 29, 2023

[RF] Add methods to create owning RooFit proxies via std::unique_ptr #12924

Merged

phsft-bot mentioned this pull request Dec 6, 2023

[RF] New RooFit::Owner pointer wrapper to flag owning return values #9392

Closed

Simplify implementation of TClass::FindClassOrBaseMethodWithId() #908

Simplify implementation of TClass::FindClassOrBaseMethodWithId() #908

Uh oh!

Conversation

amadio commented Aug 30, 2017

Uh oh!

phsft-bot commented Aug 30, 2017

Uh oh!

amadio commented Aug 30, 2017

Uh oh!

pcanal commented Aug 30, 2017

Uh oh!

amadio commented Aug 30, 2017

Uh oh!

pcanal commented Aug 30, 2017

Uh oh!

pcanal commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amadio commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

amadio commented Aug 30, 2017

Uh oh!

pcanal commented Aug 30, 2017

Uh oh!

amadio commented Aug 30, 2017

Uh oh!

pcanal commented Aug 30, 2017

Uh oh!

amadio commented Aug 30, 2017

Uh oh!

phsft-bot commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Aug 30, 2017

Uh oh!

phsft-bot commented Aug 30, 2017

Uh oh!

phsft-bot commented Aug 30, 2017

Failing tests:

Uh oh!

phsft-bot commented Aug 30, 2017

Failing tests:

Uh oh!

phsft-bot commented Aug 30, 2017

Failing tests:

Uh oh!

phsft-bot commented Aug 30, 2017

Errors:

Failing tests:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pcanal commented Aug 30, 2017 •

edited

Loading