aboutsummaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorGravatar Thomas Van Lenten <thomasvl@google.com>2015-11-17 16:48:59 -0500
committerGravatar Thomas Van Lenten <thomasvl@google.com>2015-11-17 16:48:59 -0500
commit2a91c64f496f8c137e923326a9f862e1008e26e0 (patch)
treeda04b28a4ece2c3d93b648b68574b70030633c25
parent496826efa0d1030f3ca69a52728d8c2382bbfadc (diff)
Reorder the checks so anything in the expected file is an implicit whitelisting.
In the old flow, any 2 char prefix in the expected file was still generating a warning about being a poor prefix. Now we check the expected file first, so anything expected is let through.
-rw-r--r--src/google/protobuf/compiler/objectivec/objectivec_helpers.cc101
1 files changed, 51 insertions, 50 deletions
diff --git a/src/google/protobuf/compiler/objectivec/objectivec_helpers.cc b/src/google/protobuf/compiler/objectivec/objectivec_helpers.cc
index b724d35c..990aca24 100644
--- a/src/google/protobuf/compiler/objectivec/objectivec_helpers.cc
+++ b/src/google/protobuf/compiler/objectivec/objectivec_helpers.cc
@@ -937,41 +937,17 @@ bool ValidateObjCClassPrefix(const FileDescriptor* file, string* out_error) {
// NOTE: src/google/protobuf/compiler/plugin.cc makes use of cerr for some
// error cases, so it seems to be ok to use as a back door for warnings.
- // First Check: Warning - if there is a prefix, ensure it is is a reasonable
- // value according to Apple's rules.
- if (prefix.length()) {
- if (!ascii_isupper(prefix[0])) {
- cerr << endl
- << "protoc:0: warning: Invalid 'option objc_class_prefix = \""
- << prefix << "\";' in '" << file->name() << "';"
- << " it should start with a capital letter." << endl;
- cerr.flush();
- }
- if (prefix.length() < 3) {
- cerr << endl
- << "protoc:0: warning: Invalid 'option objc_class_prefix = \""
- << prefix << "\";' in '" << file->name() << "';"
- << " Apple recommends they should be at least 3 characters long."
- << endl;
- cerr.flush();
- }
- }
-
// Load any expected package prefixes to validate against those.
map<string, string> expected_package_prefixes;
string expect_file_path;
if (!LoadExpectedPackagePrefixes(&expected_package_prefixes,
&expect_file_path, out_error)) {
- return false;
+ // Any error, clear the entries that were read.
+ expected_package_prefixes.clear();
}
- // If there are no expected prefixes, out of here.
- if (expected_package_prefixes.size() == 0) {
- return true;
- }
-
- // Second Check: Error - See if there was an expected prefix for the package
- // and report if it doesn't match.
+ // Check: Error - See if there was an expected prefix for the package and
+ // report if it doesn't match (wrong or missing).
map<string, string>::iterator package_match =
expected_package_prefixes.find(package);
if (package_match != expected_package_prefixes.end()) {
@@ -991,32 +967,57 @@ bool ValidateObjCClassPrefix(const FileDescriptor* file, string* out_error) {
}
}
- // Third Check: Error - If there was a prefix make sure it wasn't expected
- // for a different package instead (overlap is allowed, but it has to be
- // listed as an expected overlap).
- if (prefix.length()) {
- for (map<string, string>::iterator i = expected_package_prefixes.begin();
- i != expected_package_prefixes.end(); ++i) {
- if (i->second == prefix) {
- *out_error =
- "protoc:0: error: Found 'option objc_class_prefix = \"" + prefix +
- "\";' in '" + file->name() +
- "'; that prefix is already used for 'package " + i->first +
- ";'. It can only be reused by listing it in the expected file (" +
- expect_file_path + ").";
- return false; // Only report first usage of the prefix.
- }
+ // If there was no prefix option, we're done at this point.
+ if (prefix.length() == 0) {
+ // No prefix, nothing left to check.
+ return true;
+ }
+
+ // Check: Error - Make sure the prefix wasn't expected for a different
+ // package (overlap is allowed, but it has to be listed as an expected
+ // overlap).
+ for (map<string, string>::iterator i = expected_package_prefixes.begin();
+ i != expected_package_prefixes.end(); ++i) {
+ if (i->second == prefix) {
+ *out_error =
+ "protoc:0: error: Found 'option objc_class_prefix = \"" + prefix +
+ "\";' in '" + file->name() +
+ "'; that prefix is already used for 'package " + i->first +
+ ";'. It can only be reused by listing it in the expected file (" +
+ expect_file_path + ").";
+ return false; // Only report first usage of the prefix.
}
}
- // Fourth Check: Warning - If there was a prefix, and it wasn't expected,
- // issue a warning suggesting it gets added to the file.
- if (prefix.length()) {
+ // Check: Warning - Make sure the prefix is is a reasonable value according
+ // to Apple's rules (the checks above implicitly whitelist anything that
+ // doesn't meet these rules).
+ if (!ascii_isupper(prefix[0])) {
+ cerr << endl
+ << "protoc:0: warning: Invalid 'option objc_class_prefix = \""
+ << prefix << "\";' in '" << file->name() << "';"
+ << " it should start with a capital letter." << endl;
+ cerr.flush();
+ }
+ if (prefix.length() < 3) {
+ // Apple reserves 2 character prefixes for themselves. They do use some
+ // 3 character prefixes, but they haven't updated the rules/docs.
+ cerr << endl
+ << "protoc:0: warning: Invalid 'option objc_class_prefix = \""
+ << prefix << "\";' in '" << file->name() << "';"
+ << " Apple recommends they should be at least 3 characters long."
+ << endl;
+ cerr.flush();
+ }
+
+ // Check: Warning - If the given package/prefix pair wasn't expected, issue a
+ // warning issue a warning suggesting it gets added to the file.
+ if (!expected_package_prefixes.empty()) {
cerr << endl
- << "protoc:0: warning: Found 'option objc_class_prefix = \"" << prefix
- << "\";' in '" << file->name() << "';"
- << " should you add it to the expected prefixes file ("
- << expect_file_path << ")?" << endl;
+ << "protoc:0: warning: Found unexpected 'option objc_class_prefix = \""
+ << prefix << "\";' in '" << file->name() << "';"
+ << " consider adding it to the expected prefixes file ("
+ << expect_file_path << ")." << endl;
cerr.flush();
}