Skip to content

Conversation

@ferdymercury
Copy link
Collaborator

This Pull request:

Changes or fixes:

Fixes https://its.cern.ch/jira/browse/ROOT-9288

New version addressing this comment from pcanal: #17846 (comment)

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@github-actions
Copy link

Test Results

    15 files      15 suites   3d 21h 58m 33s ⏱️
 2 688 tests  2 688 ✅ 0 💤 0 ❌
39 769 runs  39 769 ✅ 0 💤 0 ❌

Results for commit c3216c6.

void AddLast(TObject *obj) override;
virtual void AddLast(TObject *obj, Option_t *opt);
void AddAt(TObject *obj, Int_t idx) override;
virtual void AddAt(TObject *obj, Int_t idx, Option_t *opt);
Copy link
Member

Choose a reason for hiding this comment

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

Rather than declaration this virtual we need to also update to TSeqCollection.h

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I was just copy pasting the structure from lines 85-86. AddLast overrides from TSeqCollection, AddLast with option is virtual. Otherwise it's not consistent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And if we also moved AddLast and AddFirst to TSeqCollection to be consistent, that would break user-scripts deriving from that classes since they would become purely virtual (if one wants to keep consistent style).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And if we made them non-purely-virtual, then users might start getting warnings such as warning: ‘virtual void ::AddAt(TObject*, Int_t, Option_t*)’ was hidden ?

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise it's not consistent?

Right. I missed that.

start getting warnings such as warning: ‘virtual void ::AddAt(TObject*, Int_t, Option_t*)’ was hidden ?

To extent that is better than use that:

  • Inherits from TList
  • Does not write/override AddAt(TObject*, Int_t, Option_t*
  • Somehow starts using it ...
  • It would 'silently' fail (depending what the overload does - but for example it would/was the case for THashList)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

v3 version here: #18389

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants