fix(generator): preserve comment introducers across embedded newlines - #16332
fix(generator): preserve comment introducers across embedded newlines#16332colinmoy wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates FormatCommentBlock in codegen_utils.cc to correctly handle multi-line comments and paragraph splitting, and adds corresponding unit tests. The review feedback recommends optimizing memory usage by using absl::string_view instead of std::string to avoid unnecessary heap allocations, and refactoring the final formatting loop to eliminate an unnecessary else block, aligning with the repository's style guide.
| std::vector<std::string> lines; | ||
| std::size_t start_pos = 0; | ||
| while (start_pos != std::string::npos) { | ||
| std::size_t boundary = start_pos + comment_width; | ||
| std::size_t end_pos = boundary; | ||
| if (boundary < comment.length()) { | ||
| // Look backward from the boundary for the last word | ||
| end_pos = comment.rfind(' ', boundary); | ||
| // If there is only one word, find and use its boundary | ||
| if (end_pos == std::string::npos || end_pos < start_pos) { | ||
| end_pos = comment.find(' ', boundary); | ||
| std::vector<absl::string_view> paragraphs = absl::StrSplit(comment, '\n'); | ||
| for (auto const& paragraph : paragraphs) { | ||
| if (paragraph.empty()) { | ||
| lines.emplace_back(""); | ||
| continue; | ||
| } | ||
| std::size_t start_pos = 0; | ||
| while (start_pos != absl::string_view::npos) { | ||
| std::size_t boundary = start_pos + comment_width; | ||
| std::size_t end_pos = boundary; | ||
| if (boundary < paragraph.length()) { | ||
| // Look backward from the boundary for the last word | ||
| end_pos = paragraph.rfind(' ', boundary); | ||
| // If there is only one word, find and use its boundary | ||
| if (end_pos == absl::string_view::npos || end_pos < start_pos) { | ||
| end_pos = paragraph.find(' ', boundary); | ||
| } | ||
| } | ||
| lines.push_back( | ||
| std::string(paragraph.substr(start_pos, end_pos - start_pos))); | ||
| start_pos = paragraph.find_first_not_of(' ', end_pos); | ||
| } | ||
| lines.push_back(comment.substr(start_pos, end_pos - start_pos)); | ||
| start_pos = comment.find_first_not_of(' ', end_pos); | ||
| } |
There was a problem hiding this comment.
Using std::vector<std::string> causes unnecessary memory allocations and copies for each line of the comment block. Since the comment parameter is passed as a const std::string& and its lifetime outlives this function, we can use std::vector<absl::string_view> instead to avoid these heap allocations.
std::vector<absl::string_view> lines;
std::vector<absl::string_view> paragraphs = absl::StrSplit(comment, '\n');
for (auto const& paragraph : paragraphs) {
if (paragraph.empty()) {
lines.emplace_back();
continue;
}
std::size_t start_pos = 0;
while (start_pos != absl::string_view::npos) {
std::size_t boundary = start_pos + comment_width;
std::size_t end_pos = boundary;
if (boundary < paragraph.length()) {
// Look backward from the boundary for the last word
end_pos = paragraph.rfind(' ', boundary);
// If there is only one word, find and use its boundary
if (end_pos == absl::string_view::npos || end_pos < start_pos) {
end_pos = paragraph.find(' ', boundary);
}
}
lines.push_back(paragraph.substr(start_pos, end_pos - start_pos));
start_pos = paragraph.find_first_not_of(' ', end_pos);
}
}References
- Scrutinize copies of non-fundamental C++ types. Is it necessary to copy the data? (link)
There was a problem hiding this comment.
use std::string_view instead of absl::string_view
f41554a to
2235e98
Compare
No description provided.