Skip to content

[core] rm C99 workaround for snprintf - #23240

Merged
guitargeek merged 6 commits into
root-project:masterfrom
ferdymercury:rmsnprintf
Sep 10, 2026
Merged

[core] rm C99 workaround for snprintf#23240
guitargeek merged 6 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

    22 files      22 suites   3d 9h 58m 53s ⏱️
 3 880 tests  3 879 ✅ 0 💤 1 ❌
76 121 runs  76 120 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 4c4cbf1.

♻️ 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
ferdymercury force-pushed the rmsnprintf branch 4 times, most recently from 7aea003 to 659726b Compare September 8, 2026 13:37
pcanal
pcanal reviewed Sep 8, 2026
View reviewed changes
Comment thread README/ReleaseNotes/v642/index.md Outdated
pcanal
pcanal requested changes Sep 8, 2026
View reviewed changes

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

I agree with this but I would at the very least leave a explicit error rather than just removing the header (unless it was never shipped as part the ROOT installation).

ferdymercury
ferdymercury commented Sep 8, 2026
View reviewed changes
Comment thread README/ReleaseNotes/v642/index.md Outdated
@ferdymercury
ferdymercury requested a review from pcanal September 8, 2026 19:04
pcanal
pcanal reviewed Sep 8, 2026
View reviewed changes
Comment thread core/clib/inc/snprintf.h
pcanal
pcanal reviewed Sep 8, 2026
View reviewed changes
Comment thread core/clib/inc/snprintf.h Outdated
@ferdymercury
ferdymercury requested a review from pcanal September 8, 2026 21:15
@ferdymercury
[core] rm C99 workaround for snprintf
9c8a2f1 
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
53fd4bd 
[foundation] rm unused DONTNEED_VSNPRINTF macro
@ferdymercury
[RN] mention snprintf header removal
1218e05
@ferdymercury
[clib] restore snprintf.h and make it emit deprecation warnings
8cae0c4 
as suggested by pcanal
@ferdymercury
[RN] update deprecation schedule
8f30bb3
@ferdymercury
[clib] error out needsnprintf defined
4c4cbf1 
[nfc] improve error message
pcanal
pcanal approved these changes Sep 8, 2026
View reviewed changes

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

Thanks

@guitargeek
guitargeek merged commit 0fca038 into root-project:master Sep 10, 2026
32 of 35 checks passed
@ferdymercury
ferdymercury deleted the rmsnprintf branch September 10, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcanal pcanal pcanal approved these changes
@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
@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.

5 participants

@ferdymercury @hageboeck @jblomer @pcanal @guitargeek