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.

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

Fix Sprintf of data handle. #2509

Merged
merged 5 commits into from
Sep 27, 2023
Merged

Fix Sprintf of data handle. #2509

merged 5 commits into from
Sep 27, 2023

Conversation

1uc
Copy link
Collaborator

@1uc 1uc commented Sep 18, 2023

If data_handle were non-trivial, then Mac CI complains that it can't pass a non-trivial type to std::snprintf. However, given the string, it seems like a bug, and we should be printing the address of a pointer.

@1uc
Copy link
Collaborator Author

1uc commented Sep 18, 2023

One could discuss if this should really print the address of a pointer, or the string representation of a data_handle.

@1uc
Copy link
Collaborator Author

1uc commented Sep 18, 2023

The context of this change is: #2499. There data_handle is no longer trivial; and CI will fail.

@azure-pipelines
Copy link

✔️ 828aec7 -> Azure artifacts URL

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #2509 (e99c1b9) into master (79eb8ad) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2509      +/-   ##
==========================================
- Coverage   61.48%   61.48%   -0.01%     
==========================================
  Files         623      623              
  Lines      119154   119159       +5     
==========================================
- Hits        73263    73259       -4     
- Misses      45891    45900       +9     
Files Coverage Δ
src/ivoc/xmenu.cpp 59.92% <0.00%> (-0.19%) ⬇️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@1uc 1uc marked this pull request as ready for review September 19, 2023 06:46
@pramodk
Copy link
Member

pramodk commented Sep 19, 2023

One could discuss if this should really print the address of a pointer, or the string representation of a data_handle.

I wonder if it’s later but @iomaganaris might know better.

@1uc
Copy link
Collaborator Author

1uc commented Sep 19, 2023

It's just a matter of picking what should be displayed in a comment in a generated HOC command/script.

@1uc
Copy link
Collaborator Author

1uc commented Sep 20, 2023

@nrnhines would you like it to print a pointer address or modernize further to make it read something like:

//  %s data_handle set to %s\n

where the first %s get substituted by data_handle::operator<<?

@nrnhines
Copy link
Member

where the first %s get substituted by data_handle::operator<<?

Let's do this.

@azure-pipelines
Copy link

✔️ d23dafa -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ 579e630 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@1uc 1uc changed the title Fix Sprintf via static_cast<double*>. Fix Sprintf of data handle. Sep 26, 2023
@azure-pipelines
Copy link

✔️ e99c1b9 -> Azure artifacts URL

@nrnhines nrnhines merged commit 82867f0 into master Sep 27, 2023
33 of 34 checks passed
@nrnhines nrnhines deleted the 1uc/fix-snprintf-data_handle branch September 27, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants