Skip to content

Conversation

portlandhodl
Copy link
Contributor

@portlandhodl portlandhodl commented Aug 18, 2023

This PR rectifies an unnecessary set of quotes delimiting the contents of StrFormatInternalBug. This is a follow up to MarcoFalke #28123 (comment). The method of action was to remove the escaped quotes that were a part of strprintf. A single functional test case was modified to reflect the new output format.

STR_INTERNAL_BUG was applied to #28123 in std::string RPCArg::ToString(const bool oneline) in rpc/util.cpp

The results can be seen below.

Previously
image

This PR
image

Additional context can be found here.
#28123 (comment)

Thank you.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 18, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@portlandhodl portlandhodl changed the title removed StrFormatInternalBug quote delimitation rpc: removed StrFormatInternalBug quote delimitation Aug 18, 2023
@portlandhodl portlandhodl marked this pull request as draft August 18, 2023 03:50
@portlandhodl portlandhodl force-pushed the 23-08-17-StrFormatInternalBug-Delimitation branch from 18cef28 to 6e8f646 Compare August 18, 2023 04:04
@portlandhodl portlandhodl marked this pull request as ready for review August 18, 2023 04:14
@maflcko
Copy link
Member

maflcko commented Aug 18, 2023

review ACK 6e8f646

Makes sense to use a single function consistently. This will now also print file, line, func of where the check is located, not where the presumed bug is located, but this seems fine, because the bug could also be in the check. In theory, there could be a flag to disable file, line, func, but that seems over-optimizing for developer-only code paths that are rarely hit.

@maflcko maflcko removed the CI failed label Aug 18, 2023
@fanquake fanquake requested a review from stickies-v August 18, 2023 14:41
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK 6e8f646

@maflcko
Copy link
Member

maflcko commented Sep 5, 2023

rfm, or is anything left to be done here?

@fanquake fanquake merged commit 337d6f3 into bitcoin:master Sep 5, 2023
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
…tion

6e8f646 removed StrFormatInternalBug quote delimitation (Reese Russell)

Pull request description:

  This PR rectifies an unnecessary set of quotes delimiting the contents of  ```StrFormatInternalBug```. This is a follow up to MarcoFalke bitcoin#28123 (comment).  The method of action was to remove the escaped quotes that were a part of strprintf. A single functional test case was modified to reflect the new output format.

  ```STR_INTERNAL_BUG``` was applied to bitcoin#28123 in ```std::string RPCArg::ToString(const bool oneline)``` in ```rpc/util.cpp```

  The results can be seen below.

  Previously
  ![image](https://github.com/bitcoin/bitcoin/assets/3104223/53f9ea59-317f-4c62-9fc1-04255eeb4641)

  This PR
  ![image](https://github.com/bitcoin/bitcoin/assets/3104223/5c6a3110-f1f3-4b3c-8e8a-9c8f1c3176e7)

  Additional context can be found here.
  bitcoin#28123 (comment)

  Thank you.

ACKs for top commit:
  MarcoFalke:
    review ACK 6e8f646
  stickies-v:
    ACK 6e8f646

Tree-SHA512: 35317e31a527630495b566407e37db9941dab7f81cfaeb1ea3309683c48e4273284645ad615f73e646a137b4f2ae35933603e9182a7dbdd22cac98d038c491dc
@bitcoin bitcoin locked and limited conversation to collaborators Sep 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants