Skip to content

[core] rm C99 workaround for snprintf - #23240

Open
ferdymercury wants to merge 1 commit into
root-project:masterfrom
ferdymercury:rmsnprintf
Open

[core] rm C99 workaround for snprintf#23240
ferdymercury wants to merge 1 commit into
root-project:masterfrom
ferdymercury:rmsnprintf

Conversation

@ferdymercury

Copy link
Copy Markdown
Collaborator

it's part of the standard since C11, so no need for special wrapper

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

Test Results

     4 files       4 suites   22h 18m 37s ⏱️
 3 795 tests  3 795 ✅ 0 💤 0 ❌
14 550 runs  14 550 ✅ 0 💤 0 ❌

Results for commit e3e44fc.

Comment thread rootx/CMakeLists.txt

@jblomer jblomer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! In principle looks good to me but the <cstdio> include should be in the include section of standard headers, below the ROOT includes.

Comment thread core/base/inc/TString.h Outdated
Comment thread core/base/src/TDatime.cxx Outdated
Comment thread core/base/src/TObject.cxx Outdated
@ferdymercury ferdymercury added the skip code analysis Skip the code analysis CI steps for this PR, including verifying clang-formatting and running Ruff. label Sep 3, 2026

@ferdymercury ferdymercury left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

mv to include section below or rm if duplicate

Comment thread core/base/inc/TString.h Outdated
Comment thread core/base/src/TColor.cxx
Comment thread core/base/src/TColor.cxx Outdated
Comment thread core/base/src/TDatime.cxx Outdated
Comment thread core/base/src/TDatime.cxx Outdated
Comment thread tree/tree/src/TChain.cxx Outdated
Comment thread tree/tree/src/TTree.cxx Outdated
Comment thread tree/tree/src/TTreeCloner.cxx Outdated
Comment thread tree/treeplayer/src/TTreeFormula.cxx Outdated
Comment thread tree/treeplayer/src/TTreePlayer.cxx Outdated

@hageboeck hageboeck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hello, I started the same here 9da0df4, but in that commit I found a few instances where the header is not needed at all.

Could we pick those few instances and remove the include altogether? I then can rebase #23150 on top of this PR, and remove the corresponding commit.

@ferdymercury ferdymercury left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

mv to include section below or rm if duplicate

@ferdymercury

Copy link
Copy Markdown
Collaborator Author

Hello, I started the same here

oops sorry I missed that

@hageboeck

hageboeck commented Sep 3, 2026

Copy link
Copy Markdown
Member

Hello, I started the same here

oops sorry I missed that

No problem, let's just remove the include in the places where it's not needed.

it's part of the standard since C11, so no need for special wrapper

use C++ header name

mv C std headers to include section below or rm duplicate
as suggested by jblomer
@ferdymercury

Copy link
Copy Markdown
Collaborator Author

No problem, let's just remove the include in the places where it's not needed.

I think I checked everywhere now :)

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

Labels

pr:squash on merge skip code analysis Skip the code analysis CI steps for this PR, including verifying clang-formatting and running Ruff.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants