Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[io] TList is not correctly reconstructed from TBuffer #17206

Open
1 task done
linev opened this issue Dec 5, 2024 · 4 comments
Open
1 task done

[io] TList is not correctly reconstructed from TBuffer #17206

linev opened this issue Dec 5, 2024 · 4 comments

Comments

@linev
Copy link
Member

linev commented Dec 5, 2024

Check duplicate issues.

  • Checked for duplicates

Description

Following list is not correctly reconstructed:

   TList lst0;
   lst0.Add(new TNamed("name0", "title0"));  // without option
   lst0.Add(new TNamed("name1", "title1"), ""); // with empty option
   lst0.Add(new TNamed("name2", "title2"), "option2"); // with non-empty option

Problem is second element - with empty option.
In the TList it is stored as TObjOptLink. One can change option and option is independent from the object itself.

If such TList read from buffer, for second entry TObjLink is created. Means option cannot be changed. And link->GetOption() returns option from the object - which can be not empty.

Both caveats lead to problems with TPad list of primitives. If TCanvas read from the file, suddenly draw option for histogram or for title may be changed - while instead reading empty option from TObjOptLink some default option from histogram or from title will be returned. Also TObject::SetDrawOption() will fail.

Related problem with JSON storage was resolved in #17198

Reproducer

TEST(TMemFile, TList_Add_Without_Option)
{
   TList lst0;
   lst0.Add(new TNamed("name0", "title0"));  // without option
   lst0.Add(new TNamed("name1", "title1"), ""); // with empty option
   lst0.Add(new TNamed("name2", "title2"), "option2"); // with non-empty option

   auto f = new TMemFile("test.root", "RECREATE");
   f->WriteTObject(&lst0, "list");

   lst0.Delete();

   auto lst1 = f->Get<TList>("list");
   delete f;

   EXPECT_NE(lst1, nullptr);

   auto link = lst1->FirstLink();

   EXPECT_STREQ("name0", link->GetObject()->GetName());
   EXPECT_STREQ("title0", link->GetObject()->GetTitle());
   EXPECT_STREQ("", link->GetAddOption()); // by default empty string returned
   EXPECT_EQ(dynamic_cast<TObjOptLink *>(link), nullptr);

   link = link->Next();

   EXPECT_STREQ("name1", link->GetObject()->GetName());
   EXPECT_STREQ("title1", link->GetObject()->GetTitle());
   EXPECT_STREQ("", link->GetAddOption());

    // this line fails
    EXPECT_NE(dynamic_cast<TObjOptLink *>(link), nullptr); 

   link = link->Next();

   EXPECT_STREQ("name2", link->GetObject()->GetName());
   EXPECT_STREQ("title2", link->GetObject()->GetTitle());
   EXPECT_STREQ("option2", link->GetAddOption());
   EXPECT_NE(dynamic_cast<TObjOptLink *>(link), nullptr);

   link = link->Next();

   EXPECT_EQ(link, nullptr);
}

ROOT version

master, but also ROOT versions for at least 16 years back are affected

Installation method

Build from source

Operating system

OpenSUSE Linux

Additional context

No response

@linev
Copy link
Member Author

linev commented Dec 5, 2024

Here list of affected classes - where GetOption() method is redefined and which suddenly can be painted different as before:

  • TGeoVolume
  • TArrow
  • TCandle
  • TGaxis
  • TLegendEntry
  • TPave
  • TPolyLine
  • TPolyMarker
  • TAxis3D, THelix, TNode, TPoints3DABC (all deprecated)
  • TPolyLine3D
  • TPolyMarker3D
  • TH1

@pcanal
Copy link
Member

pcanal commented Dec 5, 2024

This might require a version number increase for TList. The information on whether the object in the list had no option specified or a specified option of length 0 is currently lost.

@pcanal pcanal assigned dpiparo and unassigned pcanal Dec 5, 2024
@linev
Copy link
Member Author

linev commented Dec 5, 2024

Problem with version increase - such file cannot be read with older ROOT versions.
And TList used in quite many places - not only in canvas primitives

@pcanal
Copy link
Member

pcanal commented Dec 5, 2024

True. An option would also be to compare the option recorded by TList::Streamer with the one in the object as read and set it in the list if they differ (or more exactly set an empty list option if the recorded list option is empty and the object's option is not empy). This would not fix the reproducer as is since the onfile bit recorded for lst0.Add(new TNamed("name1", "title1"), ""); and lst0.Add(new TNamed("name1", "title1")); would be exactly the same but it should fix the practical issue described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants