Copilot commented on code in PR #3889:
URL: https://github.com/apache/avro/pull/3889#discussion_r3612708416
##########
lang/c++/impl/Compiler.cc:
##########
@@ -132,6 +132,24 @@ string getStringField(const Entity &e, const Object &m,
return it->second.stringValue();
}
+// Validates that a record field name or enum symbol conforms to the Avro name
+// grammar (a non-empty sequence of [A-Za-z0-9_], as already enforced for named
+// type simple names by Name::check()). This prevents out-of-spec strings from
+// being emitted verbatim as identifiers by the C++ code generator. The
character
+// checks are restricted to ASCII on purpose (rather than std::isalnum, which
is
+// locale-dependent) so the accepted grammar does not vary with the locale.
Review Comment:
validateSimpleName intentionally enforces an ASCII-only [A-Za-z0-9_] grammar
to avoid locale-dependent acceptance, but named-type simple names are still
validated via Name::check() (Node.cc) using std::isalnum, which *is*
locale-dependent. This introduces inconsistent name-validation rules (and can
still allow non-ASCII letters in named types depending on locale), undermining
the stated goal of keeping the accepted grammar locale-invariant and consistent
across schema elements.
Consider refactoring so Name::check() and validateSimpleName share the same
locale-independent ASCII predicate (ideally updating Name::check() to match),
or otherwise relax validateSimpleName to the existing Name::check() behavior
and update the comment accordingly.
--
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]
