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

Fix printable bytes that may be escaped incorrectly #5145

Merged
merged 3 commits into from
Jul 12, 2021
Merged

Fix printable bytes that may be escaped incorrectly #5145

merged 3 commits into from
Jul 12, 2021

Conversation

nblintao
Copy link
Contributor

@nblintao nblintao commented Jul 9, 2021

0x5C '\' should not be counted as normal printables

Code-Reviewer Section

The general guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

  • The PR has a description, explaining both the problem and the solution.
  • The description mentions which forms of testing were done and the testing seems reasonable.
  • Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or master if this is the youngest branch)
  • There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 79ae18f
  • Result: FAILED
  • Build Logs (available for 7 days)

alecgrieser
alecgrieser previously approved these changes Jul 9, 2021
View reviewed changes
flow/Trace.h Outdated
@@ -333,6 +333,7 @@ struct TraceableStringImpl : std::true_type {
result.push_back(base16Char(byte));
}
}
ASSERT(result.size() == result.capacity());
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this assert is worth having in the production code. Might consider making it a TEST_ASSERT (I think that's what that does anyway)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe ASSERT_WE_THINK is the relevant test macro to test something only in simulation.

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 5f7add5
  • Result: FAILED
  • Build Logs (available for 7 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: e422d42
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@jzhou77 jzhou77 merged commit 741c46f into apple:master Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-abeamon sfc-gh-abeamon sfc-gh-abeamon left review comments

@alecgrieser alecgrieser alecgrieser approved these changes

@halfprice halfprice halfprice approved these changes

@xumengpanda xumengpanda Awaiting requested review from xumengpanda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nblintao @foundationdb-ci @alecgrieser @halfprice @sfc-gh-abeamon @jzhou77