Parsing of "package a.b.c; package d.e.f;" in proto file

[Chris Kuklewicz]

In working on the Haskell version, I noticed a small oddity in "src/
google/protobuf/compiler/parser.cc".

The function to parse a package identifier in a '.proto' file is

bool Parser::ParsePackage(FileDescriptorProto* file) {
if (file->has_package()) {
AddError("Multiple package definitions.");
}

DO(Consume("package"));

RecordLocation(file, DescriptorPool::ErrorCollector::NAME);

while (true) {
string identifier;
DO(ConsumeIdentifier(&identifier, "Expected identifier."));
file->mutable_package()->append(identifier);
if (!TryConsume(".")) break;
file->mutable_package()->append(".");
}

DO(Consume(";"));
return true;
}

That looks like it would take "package a.b.c; package d.e.f;" and
result in a string that holds "a.b.cd.e.f".

Note that I have not tested this to see it this is actually the case.

The AddError tells the caller something was wrong with the '.proto'
file, so this is never used.
But I suspect it would be best to clear the string before the 'while'
loop, so the last package name is retained instead of their
concatenation. This would also become more like the semantics for
reading multiple optional fields off the wire protocol encoding.

Cheers,
Chris Kuklewicz

[Kenton Varda]

On Wed, Jul 9, 2008 at 1:08 AM, Chris Kuklewicz <turin...@gmail.com> wrote:

The AddError tells the caller something was wrong with the '.proto'
file, so this is never used.
But I suspect it would be best to clear the string before the 'while'
loop, so the last package name is retained instead of their
concatenation.  This would also become more like the semantics for
reading multiple optional fields off the wire protocol encoding.

If the Parser reports an error, then the results of parsing aren't supposed to be used anyway.  But, I agree that this might lead to other, bogus errors reported later in the file, so it might make sense to change this.