Skip to content

Emulated Collection Proxy need to always use the VectorLooper for the StreamerInfoActions#9750

Merged
pcanal merged 1 commit intoroot-project:masterfrom
pcanal:master-issue-9136
Feb 1, 2022
Merged

Emulated Collection Proxy need to always use the VectorLooper for the StreamerInfoActions#9750
pcanal merged 1 commit intoroot-project:masterfrom
pcanal:master-issue-9136

Conversation

@pcanal
Copy link
Copy Markdown
Member

@pcanal pcanal commented Jan 29, 2022

This fixes #9136.

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Jan 29, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@pcanal pcanal requested a review from eguiraud January 29, 2022 01:13
@pcanal pcanal self-assigned this Jan 29, 2022
@pcanal pcanal added the in:I/O label Jan 29, 2022
@pcanal pcanal added this to the 6.26/00 milestone Jan 29, 2022
@pcanal pcanal mentioned this pull request Jan 29, 2022
Closed
@eguiraud eguiraud requested a review from Axel-Naumann January 29, 2022 10:01
eguiraud
eguiraud approved these changes Jan 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@eguiraud eguiraud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to trust you that the new logic is better than the old 😄

Maybe the commit message could be more verbose about what was wrong before and how this patch fixes it.

Also, should we have a test for the case this fixes?

@pcanal
Copy link
Copy Markdown
Member Author

pcanal commented Jan 30, 2022

Maybe the commit message could be more verbose about what was wrong before and how this patch fixes it.

Fair enough, I will expand.

Also, should we have a test for the case this fixes?

Yes :) Can you add the test you case you had?

@pcanal
Emulated Collection Proxy always need the VectorLooper for the Stream…
68d37bc 
…erInfoActions

This fixes root-project#9136.

Without this commit, SelectLooper would select the 'GenericLooper'
in the case of an emulated proxy for STL collection with (in the
name) a custom allocator.  However the GenericLooper only usable
for collection with a compiled collection proxy.

In particular, GenericLooper is calling the 'Next' function which
is not defined for vector ... and emulated collection.  Using
it lead to an assert complaining (right fully so) that an
'undefined' function is being called.
@pcanal pcanal force-pushed the master-issue-9136 branch from dfcf963 to 68d37bc Compare January 30, 2022 19:46
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Jan 30, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@Axel-Naumann
Copy link
Copy Markdown
Member

Axel-Naumann commented Jan 31, 2022 •
edited
Loading

Yes :) Can you add the test you case you had?

That needed valgrind to detect anything. @pcanal can we use TStreamerInfo::ls() or something to validate the layout? Like this (output corresponds to master)?

root [0] TClass::GetClass("std::map<int, std::set<float>>")->GetStreamerInfo()->ls()

StreamerInfo for class: map<int,set<float> >, version=6, checksum=0xbe69c367
  map<int,set<float> > This            offset=  0 type=300 ,stl=4, ctype=61, <pair<int,set<float> >> Used to call the proper TStreamerInfo case
   i= 0, This            type=300, offset=  0, len=1, method=0

@pcanal
Copy link
Copy Markdown
Member Author

pcanal commented Jan 31, 2022

No valgrind necessary. The failure is a call to R__ASSERT. There is not really any other means to inspect that.

@Axel-Naumann
Copy link
Copy Markdown
Member

Axel-Naumann commented Jan 31, 2022

OK, I don't think we should wait for someone to add a test (unless it's you @pcanal ). Please merge this especially into v6-26...

@pcanal pcanal merged commit 5d69eca into root-project:master Feb 1, 2022
@pcanal pcanal deleted the master-issue-9136 branch February 1, 2022 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Axel-Naumann Axel-Naumann Awaiting requested review from Axel-Naumann

1 more reviewer

@eguiraud eguiraud eguiraud approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@pcanal pcanal

Labels

in:I/O

Projects

None yet

Milestone

6.26/00

Development

Successfully merging this pull request may close these issues.

[I/O] Cannot read RVecs written with v6.24 with TTreeReader in current master

4 participants

@pcanal @phsft-bot @Axel-Naumann @eguiraud