Can one of the admins verify this patch?

Backward-incompatible. @pcanal leaving it up as a reminder to decide what to do with #8300

@pcanal I have thought of a "solution". If we remove the default value, set event to 0 in the method itself and simply warn the user that he/she has not provided a value for event, the user will know and other classes that rely on the default value of 0 still work.

I am not sure how this would work. Can you make a concrete (code) proposal?

I am not sure how this would work. Can you make a concrete (code) proposal?

I had something like this in mind:

So the declarations in TTree.h remain unchanged, i.e. 0 is the default value for ´entry´

However, by adding this line to both GetEntry() and GetEvent() in TTree.cxx:
if (!event) Warning("TTree::GetEvent", "No value for event providied. Set to default value 0");
The user gets notified if he forgets to provide a value for ´event but other classes that rely on ´event´ being the default value of 0 work as they did before. Would this work?

Well, wouldn't that issue a spurrious/wrong warning in the following case (i.e the normal usage):

for(Long64_t entry = 0; entry < tree->GetEntriesFast(); ++entry)
{
     tree->GetEntry(entry);
}

[Slightly related question, did you run any of the test after successfully compiling?]

@phsft-bot build just on ROOT-performance-centos8-multicore/default

Starting build on ROOT-performance-centos8-multicore/default
How to customize builds

Well, wouldn't that issue a spurrious/wrong warning in the following case (i.e the normal usage):

for(Long64_t entry = 0; entry < tree->GetEntriesFast(); ++entry)
{
     tree->GetEntry(entry);
}

[Slightly related question, did you run any of the test after successfully compiling?]

Actually I had some trouble compiling in my usual manor. But when I tried to compile upstream/master it also failed so the failure didn't seem to originate from my chnages. I wasnt sure what to do so I was hoping I could see what phsft-bot has to say before I do anything.

This is how I build: (Ubuntu 20.04 LTS)

mkdir rootbuild rootinstall 
cd rootbuild
cmake -DCMAKE_INSTALL_PREFIX=../rootinstall/ ../root/
cmake --build . -- -j4
cd bin
source thisroot.sh
root

Well, wouldn't that issue a spurrious/wrong warning in the following case (i.e the normal usage):

for(Long64_t entry = 0; entry < tree->GetEntriesFast(); ++entry)
{
     tree->GetEntry(entry);
}

[Slightly related question, did you run any of the test after successfully compiling?]

But here the user would be providing a value for entry that is 0. The warning would only trigger if entry is not provided at all, i.e. !entry is true.

!entry == true if entry == 0: 0 is fals-y in C++

!entry == true if entry == 0: 0 is fals-y in C++

Ohh I didn't know that. Then maybe one could simply check if entry is undefined. Or does that cause the same error?

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

• projectroot.runtutorials.tutorial_unfold_testUnfold7b
• projectroot.runtutorials.tutorial_tmva_TMVAMultipleBackgroundExample
• projectroot.runtutorials.tutorial_tmva_TMVARegression
• projectroot.runtutorials.tutorial_tmva_TMVAClassification
• projectroot.runtutorials.tutorial_tmva_TMVAClassificationCategory
• projectroot.runtutorials.tutorial_unfold_testUnfold5c
• projectroot.runtutorials.tutorial_io_hadd
• projectroot.runtutorials.tutorial_legacy_mlp_mlpRegression
• projectroot.runtutorials.tutorial_legacy_mlp_mlpHiggs
• projectroot.runtutorials.tutorial_math_mathcoreVectorCollection

And 141 more

Is this because 0 is no longer the default value?

There is no "undefined" value for an integer variable. The test failures are because with these changes we are printing errors that were not printed before.

There is no "undefined" value for an integer variable. The test failures are because with these changes we are printing errors that were not printed before.

Ah ok

Please remove the if (!entry) test and associated warning.
Please also change the signature of GetEvent.

okay. But I did some internet research and couldn't find a way to check if no entry parameter was given by the user without also raising the error when the user enters 0 (since !entry == true if entry == 0)

If we wanted to catch those cases, we would just add a new overload

Int_t GetEntry() { 
   Warning("TTree::GetEvent", "No value for event providied. Set to default value 0");
   return GetEntry(0);
}

but I don't think it is necessary/needed.

@phsft-bot build

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

Build seems to have succeeded.

Thank you very much!

Thank you very much!

Thank you for your help. I learn something new with every PR :D

Final note: In the commits left on the branch there was a "merge" commit, we want to avoid them. When updating your branch with the content of the master branch, please use 'git rebase' rather than git merge. Also we tend to prefer to simplify the history by keeping only the effective commits. [For example, in this case, you could (have done)/do git rebase -i to remove the commit that was reverse and its reversal :)].

For this PR, I effectively handled these changes by doing a "merge and squash" but this works out only for PR that have one effective commit (the case here).

Final note: In the commits left on the branch there was a "merge" commit, we want to avoid them. When updating your branch with the content of the master branch, please use 'git rebase' rather than git merge. Also we tend to prefer to simplify the history by keeping only the effective commits. [For example, in this case, you could (have done)/do git rebase -i to remove the commit that was reverse and its reversal :)].

For this PR, I effectively handled these changes by doing a "merge and squash" but this works out only for PR that have one effective commit (the case here).

Ah ok. I recently learned what rebase -i is, so I'll use that next time. Thanks for the tip

@AdvaitDhingra This is a breaking change (we want it, it's still a breaking change). Could you add a note in README/ReleaseNotes/v626/index.md, section ## Deprecation and Removal (which you could - please - rename to ## Deprecation, Removal, Backward Incompatibilities

@AdvaitDhingra This is a breaking change (we want it, it's still a breaking change). Could you add a note in README/ReleaseNotes/v626/index.md, section ## Deprecation and Removal (which you could - please - rename to ## Deprecation, Removal, Backward Incompatibilities

Would that require a separate PR?

Would that require a separate PR?

Yes, please!

Would that require a separate PR?

Yes, please!

Okay I'll get started :D

@Axel-Naumann can I also add my name to the list of contributors?

@Axel-Naumann can I also add my name to the list of contributors?

We really appreciate small fixes like those, but we generally only call out the authors of larger / continuous contributions. We'll think of another way to show our appreciation! :-)

@Axel-Naumann can I also add my name to the list of contributors?

We really appreciate small fixes like those, but we generally only call out the authors of larger / continuous contributions. We'll think of another way to show our appreciation! :-)

Ah ok, that makes sense 😅. I'll make the PR now.