Skip to content

[core] rm C99 workaround for snprintf - #23240

Open
ferdymercury wants to merge 3 commits into
root-project:masterfrom
ferdymercury:rmsnprintf
Open

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

Conversation

@ferdymercury

@ferdymercury ferdymercury commented Sep 2, 2026

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 •
edited
Loading

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 19h 31m 8s ⏱️
 3 868 tests  3 866 ✅ 0 💤 2 ❌
79 724 runs  79 720 ✅ 1 💤 3 ❌

For more details on these failures, see this check.

Results for commit 48ef7a4.

♻️ This comment has been updated with latest results.

ferdymercury
ferdymercury commented Sep 3, 2026
View reviewed changes
Comment thread rootx/CMakeLists.txt
@ferdymercury
ferdymercury marked this pull request as ready for review September 3, 2026 06:47
jblomer
jblomer reviewed Sep 3, 2026
View reviewed changes

@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 commented Sep 3, 2026
View reviewed changes

@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 reviewed Sep 3, 2026
View reviewed changes

@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 commented Sep 3, 2026
View reviewed changes

@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

ferdymercury commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Hello, I started the same here

oops sorry I missed that

@hageboeck

hageboeck commented Sep 3, 2026 •
edited
Loading

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.

@couet
couet removed their request for review September 3, 2026 08:47
@ferdymercury

ferdymercury commented Sep 3, 2026

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 :)

hageboeck
hageboeck reviewed Sep 4, 2026
View reviewed changes

@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.

Very nice initiative!

If we remove snprintf.h, we should also clean up this (and below in the same file):

root/core/foundation/inc/ROOT/RConfig.hxx

Lines 65 to 67 in b6f36a1

# ifdef R__HPUX10
# define NEED_SNPRINTF
# endif

Finally, we should probably leave a note in the release notes that this header disappears from ROOT, and that people should use <cstdio>. What do you think?

Comment thread gui/gui/src/TRootBrowserLite.cxx Outdated
@hageboeck

hageboeck commented Sep 4, 2026

Copy link
Copy Markdown
Member

If we remove snprintf.h, we should also clean up this (and below in the same file):

root/core/foundation/inc/ROOT/RConfig.hxx

Lines 65 to 67 in b6f36a1

# ifdef R__HPUX10
# define NEED_SNPRINTF
# endif

Sorry that I didn't point this out immediately, but there's also the opposite DONTNEED_VSNPRINTF in the same file, which is always set since Visual Studio 2005.

hageboeck
hageboeck approved these changes Sep 4, 2026
View reviewed changes

@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.

Looks good from my side!

What do you think @jblomer?

@bellenot, does this look OK for Windows?

@ferdymercury ferdymercury added the clean build Ask CI to do non-incremental build on PR label Sep 5, 2026
@ferdymercury
[core] rm C99 workaround for snprintf
c214d6d 
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
[core] rm no longer used macro snprintf
409b68e 
[foundation] rm unused DONTNEED_VSNPRINTF macro
@ferdymercury
[RN] mention snprintf header removal
900ca2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hageboeck hageboeck hageboeck approved these changes
@bellenot bellenot Awaiting requested review from bellenot bellenot is a code owner
@linev linev Awaiting requested review from linev linev is a code owner
@vepadulano vepadulano Awaiting requested review from vepadulano vepadulano is a code owner
@pcanal pcanal Awaiting requested review from pcanal pcanal is a code owner
@dpiparo dpiparo Awaiting requested review from dpiparo dpiparo is a code owner
@guitargeek guitargeek Awaiting requested review from guitargeek guitargeek is a code owner
@jblomer jblomer Awaiting requested review from jblomer jblomer is a code owner

Assignees

@ferdymercury ferdymercury

Labels

clean build Ask CI to do non-incremental build on PR 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ferdymercury @hageboeck @jblomer