AAPT2: Update ReplacePlaceholder for artifact name parser
Update the artifact name parser to ensure that duplicate placeholders
are treated as an error.
Also applied suggested changes from ag/2447777.
Test: ran unit tests
Change-Id: Iab8fd9d9b81aa3008177141256ecd16ef04b0c34
diff --git a/tools/aapt2/configuration/ConfigurationParser.cpp b/tools/aapt2/configuration/ConfigurationParser.cpp
index d051120..c56492c 100644
--- a/tools/aapt2/configuration/ConfigurationParser.cpp
+++ b/tools/aapt2/configuration/ConfigurationParser.cpp
@@ -118,21 +118,33 @@
static bool ReplacePlaceholder(const std::string& placeholder, const Maybe<std::string>& value,
std::string* name, IDiagnostics* diag) {
size_t offset = name->find(placeholder);
- if (value) {
- if (offset == std::string::npos) {
+ bool found = (offset != std::string::npos);
+
+ // Make sure the placeholder was present if the desired value is present.
+ if (!found) {
+ if (value) {
diag->Error(DiagMessage() << "Missing placeholder for artifact: " << placeholder);
return false;
}
- name->replace(offset, placeholder.length(), value.value());
return true;
}
+ DCHECK(found) << "Missing return path for placeholder not found";
+
// Make sure the placeholder was not present if the desired value was not present.
- bool result = (offset == std::string::npos);
- if (!result) {
+ if (!value) {
diag->Error(DiagMessage() << "Placeholder present but no value for artifact: " << placeholder);
+ return false;
}
- return result;
+
+ name->replace(offset, placeholder.length(), value.value());
+
+ // Make sure there was only one instance of the placeholder.
+ if (name->find(placeholder) != std::string::npos) {
+ diag->Error(DiagMessage() << "Placeholder present multiple times: " << placeholder);
+ return false;
+ }
+ return true;
}
Maybe<std::string> Artifact::ToArtifactName(const std::string& format, IDiagnostics* diag) const {
diff --git a/tools/aapt2/configuration/ConfigurationParser_test.cpp b/tools/aapt2/configuration/ConfigurationParser_test.cpp
index fb71e98..ab3b7ec 100644
--- a/tools/aapt2/configuration/ConfigurationParser_test.cpp
+++ b/tools/aapt2/configuration/ConfigurationParser_test.cpp
@@ -418,6 +418,8 @@
ASSERT_THAT(out, ElementsAre(low_latency, pro));
}
+// Artifact name parser test cases.
+
TEST(ArtifactTest, Simple) {
StdErrDiagnostics diag;
Artifact x86;
@@ -468,5 +470,53 @@
EXPECT_TRUE(artifact.ToArtifactName("something.apk", &diag));
}
+TEST(ArtifactTest, Repeated) {
+ StdErrDiagnostics diag;
+ Artifact artifact;
+ artifact.screen_density_group = {"mdpi"};
+
+ EXPECT_TRUE(artifact.ToArtifactName("something.{density}.apk", &diag));
+ EXPECT_FALSE(artifact.ToArtifactName("something.{density}.{density}.apk", &diag));
+}
+
+TEST(ArtifactTest, Nesting) {
+ StdErrDiagnostics diag;
+ Artifact x86;
+ x86.abi_group = {"x86"};
+
+ EXPECT_FALSE(x86.ToArtifactName("something.{abi{density}}.apk", &diag));
+
+ const Maybe<std::string>& name = x86.ToArtifactName("something.{abi{abi}}.apk", &diag);
+ EXPECT_TRUE(name);
+ EXPECT_EQ(name.value(), "something.{abix86}.apk");
+}
+
+TEST(ArtifactTest, Recursive) {
+ StdErrDiagnostics diag;
+ Artifact artifact;
+ artifact.device_feature_group = {"{gl}"};
+ artifact.gl_texture_group = {"glx1"};
+
+ EXPECT_FALSE(artifact.ToArtifactName("app.{feature}.{gl}.apk", &diag));
+
+ artifact.device_feature_group = {"df1"};
+ artifact.gl_texture_group = {"{feature}"};
+ {
+ const auto& result = artifact.ToArtifactName("app.{feature}.{gl}.apk", &diag);
+ EXPECT_TRUE(result);
+ EXPECT_EQ(result.value(), "app.df1.{feature}.apk");
+ }
+
+ // This is an invalid case, but should be the only possible case due to the ordering of
+ // replacement.
+ artifact.device_feature_group = {"{gl}"};
+ artifact.gl_texture_group = {"glx1"};
+ {
+ const auto& result = artifact.ToArtifactName("app.{feature}.apk", &diag);
+ EXPECT_TRUE(result);
+ EXPECT_EQ(result.value(), "app.glx1.apk");
+ }
+}
+
} // namespace
} // namespace aapt