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

Improve yaml log escaping #4080

Merged
merged 2 commits into from
Nov 17, 2023
Merged

Improve yaml log escaping #4080

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ee4682
logging: improve escaping in yaml output
joennlae Nov 14, 2023
fdbef3b
logging: include review feedback
joennlae Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions common/common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1188,6 +1188,7 @@ void dump_string_yaml_multiline(FILE * stream, const char * prop_name, const cha
if (!data_str.empty() && (std::isspace(data_str[0]) || std::isspace(data_str.back()))) {
data_str = std::regex_replace(data_str, std::regex("\n"), "\\n");
data_str = std::regex_replace(data_str, std::regex("\""), "\\\"");
data_str = std::regex_replace(data_str, std::regex("\\\\[^n\"]"), "\\$&");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think raw string literals would be clearer here. And I think it should be ordered like this so a \n in the input is correctly escaped as \\n (correct me if I'm wrong):

Suggested change
data_str = std::regex_replace(data_str, std::regex("\n"), "\\n");
data_str = std::regex_replace(data_str, std::regex("\""), "\\\"");
data_str = std::regex_replace(data_str, std::regex("\\\\[^n\"]"), "\\$&");
data_str = std::regex_replace(data_str, std::regex(R"(\\)"), R"(\\)");
data_str = std::regex_replace(data_str, std::regex(R"(\n)"), R"(\n)");
data_str = std::regex_replace(data_str, std::regex(R"(")"), R"(\")");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback :-)

I see your suggestion regarding the raw string :-) I agree it makes it less confusing, but on the other side, there is also some mind gymnastics involved in understanding why we need to do the:

data_str = std::regex_replace(data_str, std::regex(R"(\n)"), R"(\n)");

replacing the same? (I mean, it is because the regex string needs the escaping for that matching, but is that obvious for the casual code reader 😄 )

I included your proposal to use raw strings, and thank you for the feedback.

data_str = "\"" + data_str + "\"";
fprintf(stream, "%s: %s\n", prop_name, data_str.c_str());
return;
Expand Down