Change: support passing std::string directly to DEBUG #9212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This approach does nog really seem to work nicely as it requires dropping format warnings from the compiler. master...rubidium42:debug_stream is another approach that uses a std::stringstream, though that clutters up the code a lot. Maybe the better approach would be to go from C-printf towards std::format, however the latter is C++20. There is also a fmt::format that is almost compatible with the std::format specification (https://fmt.dev/7.1.3/api.html#compatibility-with-c-20-std-format), so going with fmt::format might be a good interim solution to get the conversion going. Then, when we start requiring C++20, we can drop fmt and replace fmt:: with std::. So the real question is, what approach should we take? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / Problem
Many changes/conversions changing variables to std::string end up with changing calls to
DEBUGto add.c_str().This problem compounded by MSVC not complaining about this, and it even just seeming to work out of the box. So you might only find out the forgotten
.c_str()s when having created the PR.Description
Add effectively a C++-wrapper around the existing
debugthat performs the conversion where needed, and letDEBUGuse that.Limitations
Slightly more code in the debug header, but fewer chances on mistakes and fewer changes required when converting towards std::string. For now the conversion functions are in the debug header, though they should probably go to a different location once this functionality gets used in more places.
Furthermore
WARN_FORMATcannot look at the parameters anymore, so we'd be losing that bit of help. So the question is what's worse, not having that warning orc_str()in many places.Future work
Other printf style functions could be converted, though there it cannot be injected as easily as the functions are called directly. In other words, the existing functions need to be renamed and then the wrapper can be added.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.