Copilot commented on code in PR #3786:
URL: https://github.com/apache/thrift/pull/3786#discussion_r3971024392
##########
compiler/cpp/src/thrift/thriftl.ll:
##########
@@ -317,7 +325,12 @@ literal_begin (['\"])
result.push_back('\\');
continue;
default:
- yyerror("Bad escape character\n");
+ if (ch >= 0x20 && ch <= 0x7e) {
+ yyerror("Invalid escape sequence '\\%c'. Use \\\\ for a literal
backslash.\n", ch);
+ } else {
+ yyerror("Invalid escape byte 0x%02X. Use \\\\ for a literal
backslash.\n",
+ static_cast<unsigned int>(ch));
+ }
Review Comment:
For byte-oriented diagnostics, it’s safer to treat `ch` as an unsigned byte
when doing the printable-range check and when formatting hex. If `ch` were ever
negative (e.g., due to signedness/implementation details), the range check and
`0x%02X` formatting could produce incorrect results. Consider converting once
to an `unsigned char`/`uint8_t` (or an `unsigned int` derived from
`static_cast<unsigned char>(ch)`) and using that for both the range check and
hex rendering.
##########
compiler/cpp/tests/thrift_test_parser_support.cc:
##########
@@ -43,14 +43,29 @@ extern std::vector<std::string> g_incl_searchpath;
// Error reporting used by the parser.
void yyerror(const char* fmt, ...) {
- std::fprintf(stderr, "[ERROR:%s:%d] ", g_curpath.c_str(), yylineno);
va_list args;
va_start(args, fmt);
- std::vfprintf(stderr, fmt, args);
+ va_list size_args;
+ va_copy(size_args, args);
+ const int size = std::vsnprintf(nullptr, 0, fmt, size_args);
+ va_end(size_args);
+ if (size < 0) {
+ va_end(args);
+ throw std::runtime_error("Unable to format thrift parser error");
+ }
+ std::vector<char> message(size + 1);
+ const int written = std::vsnprintf(message.data(), message.size(), fmt,
args);
va_end(args);
- std::fprintf(stderr, "\n");
+ if (written != size) {
+ throw std::runtime_error("Unable to format thrift parser error");
+ }
+ std::fprintf(stderr, "[ERROR:%s:%d] ", g_curpath.c_str(), yylineno);
+ std::fwrite(message.data(), 1, static_cast<size_t>(size), stderr);
+ if (size == 0 || message[size - 1] != '\n') {
+ std::fprintf(stderr, "\n");
+ }
- throw std::runtime_error("thrift parser error");
+ throw std::runtime_error(std::string(message.data(),
static_cast<size_t>(size)));
Review Comment:
The newline normalization is applied only to the stderr output, not to the
thrown exception message. This can make the exception text differ from the
printed diagnostic when `fmt` does not end with `\\n` (stderr gets a newline,
exception doesn’t). Consider normalizing once (e.g., append `\\n` to the
formatted buffer when missing) and using the same normalized text for both
`fwrite` and the exception to keep behavior consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
