Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

clang warning about deprecated sprintf usage #4747

Merged
merged 2 commits into from Jan 16, 2024
Merged

clang warning about deprecated sprintf usage #4747

merged 2 commits into from Jan 16, 2024

Conversation

ckeshava
Copy link
Collaborator

@ckeshava ckeshava commented Oct 4, 2023

High Level Overview of Change

This fix is intended to remove a warning from the clang compiler. It switches the usage of sprintf function into the recommended snprintf function.

I observed this issue in this environment:

Apple clang version 15.0.0 (clang-1500.0.40.1)
Target: arm64-apple-darwin22.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Fix #4569

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

@ckeshava ckeshava mentioned this pull request Oct 4, 2023
Closed
7 tasks
@ckeshava
Copy link
Collaborator Author

ckeshava commented Oct 4, 2023

@intelliot @a-noni-mousse @seelabs
I have grabbed the relevant suggestions from the old PR into this pull request.

HowardHinnant
HowardHinnant approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

Looks good!

Bronek
Bronek approved these changes Dec 4, 2023
View reviewed changes
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

LGTM

@intelliot intelliot added this to the 2.0.1 (Jan 2024) milestone Dec 7, 2023
scottschurr
scottschurr approved these changes Dec 12, 2023
View reviewed changes
Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

👍 Change looks good. The changed line is also exercised by the unit tests, which increases my confidence..

src/ripple/json/impl/json_reader.cpp
char buffer[n];
snprintf(buffer, n, "Line %d, Column %d", line, column);
return buffer;
return "Line " + std::to_string(line) + ", Column " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice that you kept the form of the string formatting similar to the other string formatting in the file.

@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jan 5, 2024
@intelliot
Copy link
Collaborator

  • removes a warning. Perf attention not needed. cc @sophiax851

@intelliot intelliot added the Perf impact not expected Change is not expected to improve nor harm performance. label Jan 9, 2024
@intelliot intelliot merged commit 5a7af5b into XRPLF:develop Jan 16, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bronek Bronek Bronek approved these changes

@HowardHinnant HowardHinnant HowardHinnant approved these changes

@scottschurr scottschurr scottschurr approved these changes

@seelabs seelabs Awaiting requested review from seelabs

Assignees
No one assigned
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Perf impact not expected Change is not expected to improve nor harm performance.
Projects
Core Ledger
Status: 🚢 Released in 2.0.1
Milestone
2.0.1
Development

Successfully merging this pull request may close these issues.

Clang warning: deprecated sprintf usage in json_reader (Version: 1.11)
6 participants
@ckeshava @intelliot @Bronek @HowardHinnant @scottschurr @seelabs