AAPT2: Fix inclusion of comments in R.java javadoc

Comments weren't being copied when merged from the various
resource tables.

Also refactored the JavaClassGenerator to omit a class
if no entries exist for it.

Change-Id: I6eaa89b7b3715bc05403635a2baf0d1db3efd142

tools/aapt2/Android.mk

@@ -80,6 +80,7 @@ testSources := \
 	util/StringPiece_test.cpp \
 	util/Util_test.cpp \
 	ConfigDescription_test.cpp \
+	java/AnnotationProcessor_test.cpp \
 	java/JavaClassGenerator_test.cpp \
 	java/ManifestClassGenerator_test.cpp \
 	Locale_test.cpp \

tools/aapt2/ResourceParser.cpp

@@ -973,7 +973,7 @@ bool ResourceParser::parseDeclareStyleable(XmlPullParser* parser, ParsedResource
     const Source source = mSource.withLine(parser->getLineNumber());
     std::unique_ptr<Styleable> styleable = util::make_unique<Styleable>();
 
-    // Declare-styleable is always public, because it technically only exists in R.java.
+    // Declare-styleable is kPrivate by default, because it technically only exists in R.java.
     outResource->symbolState = SymbolState::kPublic;
 
     std::u16string comment;

tools/aapt2/ResourceTable.cpp

@@ -284,7 +284,7 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI
     }
 
     const auto endIter = entry->values.end();
-    auto iter = std::lower_bound(entry->values.begin(), endIter, config, cmp::lessThan);
+    auto iter = std::lower_bound(entry->values.begin(), endIter, config, cmp::lessThanConfig);
     if (iter == endIter || iter->config != config) {
         // This resource did not exist before, add it.
         entry->values.insert(iter, ResourceConfigValue{ config, std::move(value) });

tools/aapt2/ResourceValues.cpp

@@ -44,7 +44,10 @@ RawString::RawString(const StringPool::Ref& ref) : value(ref) {
 }
 
 RawString* RawString::clone(StringPool* newPool) const {
-    return new RawString(newPool->makeRef(*value));
+    RawString* rs = new RawString(newPool->makeRef(*value));
+    rs->mComment = mComment;
+    rs->mSource = mSource;
+    return rs;
 }
 
 bool RawString::flatten(android::Res_value* outValue) const {
@@ -77,6 +80,8 @@ bool Reference::flatten(android::Res_value* outValue) const {
 
 Reference* Reference::clone(StringPool* /*newPool*/) const {
     Reference* ref = new Reference();
+    ref->mComment = mComment;
+    ref->mSource = mSource;
     ref->referenceType = referenceType;
     ref->name = name;
     ref->id = id;
@@ -111,7 +116,10 @@ bool Id::flatten(android::Res_value* out) const {
 }
 
 Id* Id::clone(StringPool* /*newPool*/) const {
-    return new Id();
+    Id* id = new Id();
+    id->mComment = mComment;
+    id->mSource = mSource;
+    return id;
 }
 
 void Id::print(std::ostream* out) const {
@@ -133,7 +141,10 @@ bool String::flatten(android::Res_value* outValue) const {
 }
 
 String* String::clone(StringPool* newPool) const {
-    return new String(newPool->makeRef(*value));
+    String* str = new String(newPool->makeRef(*value));
+    str->mComment = mComment;
+    str->mSource = mSource;
+    return str;
 }
 
 void String::print(std::ostream* out) const {
@@ -154,7 +165,10 @@ bool StyledString::flatten(android::Res_value* outValue) const {
 }
 
 StyledString* StyledString::clone(StringPool* newPool) const {
-    return new StyledString(newPool->makeRef(value));
+    StyledString* str = new StyledString(newPool->makeRef(value));
+    str->mComment = mComment;
+    str->mSource = mSource;
+    return str;
 }
 
 void StyledString::print(std::ostream* out) const {
@@ -175,7 +189,10 @@ bool FileReference::flatten(android::Res_value* outValue) const {
 }
 
 FileReference* FileReference::clone(StringPool* newPool) const {
-    return new FileReference(newPool->makeRef(*path));
+    FileReference* fr = new FileReference(newPool->makeRef(*path));
+    fr->mComment = mComment;
+    fr->mSource = mSource;
+    return fr;
 }
 
 void FileReference::print(std::ostream* out) const {
@@ -197,7 +214,10 @@ bool BinaryPrimitive::flatten(android::Res_value* outValue) const {
 }
 
 BinaryPrimitive* BinaryPrimitive::clone(StringPool* /*newPool*/) const {
-    return new BinaryPrimitive(value);
+    BinaryPrimitive* bp = new BinaryPrimitive(value);
+    bp->mComment = mComment;
+    bp->mSource = mSource;
+    return bp;
 }
 
 void BinaryPrimitive::print(std::ostream* out) const {
@@ -236,6 +256,8 @@ bool Attribute::isWeak() const {
 
 Attribute* Attribute::clone(StringPool* /*newPool*/) const {
     Attribute* attr = new Attribute(weak);
+    attr->mComment = mComment;
+    attr->mSource = mSource;
     attr->typeMask = typeMask;
     std::copy(symbols.begin(), symbols.end(), std::back_inserter(attr->symbols));
     return attr;
@@ -358,6 +380,8 @@ Style* Style::clone(StringPool* newPool) const {
     Style* style = new Style();
     style->parent = parent;
     style->parentInferred = parentInferred;
+    style->mComment = mComment;
+    style->mSource = mSource;
     for (auto& entry : entries) {
         style->entries.push_back(Entry{
                 entry.key,
@@ -390,6 +414,8 @@ static ::std::ostream& operator<<(::std::ostream& out, const Style::Entry& value
 
 Array* Array::clone(StringPool* newPool) const {
     Array* array = new Array();
+    array->mComment = mComment;
+    array->mSource = mSource;
     for (auto& item : items) {
         array->items.emplace_back(std::unique_ptr<Item>(item->clone(newPool)));
     }
@@ -404,6 +430,8 @@ void Array::print(std::ostream* out) const {
 
 Plural* Plural::clone(StringPool* newPool) const {
     Plural* p = new Plural();
+    p->mComment = mComment;
+    p->mSource = mSource;
     const size_t count = values.size();
     for (size_t i = 0; i < count; i++) {
         if (values[i]) {
@@ -423,6 +451,8 @@ static ::std::ostream& operator<<(::std::ostream& out, const std::unique_ptr<Ite
 
 Styleable* Styleable::clone(StringPool* /*newPool*/) const {
     Styleable* styleable = new Styleable();
+    styleable->mComment = mComment;
+    styleable->mSource = mSource;
     std::copy(entries.begin(), entries.end(), std::back_inserter(styleable->entries));
     return styleable;
 }

tools/aapt2/ResourceValues.h

@@ -91,7 +91,7 @@ struct Value {
      */
     virtual void print(std::ostream* out) const = 0;
 
-private:
+protected:
     Source mSource;
     std::u16string mComment;
 };

tools/aapt2/java/AnnotationProcessor.cpp

@@ -25,33 +25,20 @@ void AnnotationProcessor::appendCommentLine(const std::string& comment) {
     static const std::string sDeprecated = "@deprecated";
     static const std::string sSystemApi = "@SystemApi";
 
-    if (comment.find(sDeprecated) != std::string::npos && !mDeprecated) {
-        mDeprecated = true;
-        if (!mAnnotations.empty()) {
-            mAnnotations += "\n";
-        }
-        mAnnotations += mPrefix;
-        mAnnotations += "@Deprecated";
+    if (comment.find(sDeprecated) != std::string::npos) {
+        mAnnotationBitMask |= kDeprecated;
     }
 
-    if (comment.find(sSystemApi) != std::string::npos && !mSystemApi) {
-        mSystemApi = true;
-        if (!mAnnotations.empty()) {
-            mAnnotations += "\n";
-        }
-        mAnnotations += mPrefix;
-        mAnnotations += "@android.annotations.SystemApi";
+    if (comment.find(sSystemApi) != std::string::npos) {
+        mAnnotationBitMask |= kSystemApi;
     }
 
-    if (mComment.empty()) {
-        mComment += mPrefix;
-        mComment += "/**";
+    if (!mHasComments) {
+        mHasComments = true;
+        mComment << "/**";
     }
 
-    mComment += "\n";
-    mComment += mPrefix;
-    mComment += " * ";
-    mComment += std::move(comment);
+    mComment << "\n" << " * " << std::move(comment);
 }
 
 void AnnotationProcessor::appendComment(const StringPiece16& comment) {
@@ -73,17 +60,22 @@ void AnnotationProcessor::appendComment(const StringPiece& comment) {
     }
 }
 
-std::string AnnotationProcessor::buildComment() {
-    if (!mComment.empty()) {
-        mComment += "\n";
-        mComment += mPrefix;
-        mComment += " */";
+void AnnotationProcessor::writeToStream(std::ostream* out, const StringPiece& prefix) {
+    if (mHasComments) {
+        std::string result = mComment.str();
+        for (StringPiece line : util::tokenize<char>(result, '\n')) {
+           *out << prefix << line << "\n";
+        }
+        *out << prefix << " */" << "\n";
     }
-    return std::move(mComment);
-}
 
-std::string AnnotationProcessor::buildAnnotations() {
-    return std::move(mAnnotations);
+    if (mAnnotationBitMask & kDeprecated) {
+        *out << prefix << "@Deprecated\n";
+    }
+
+    if (mAnnotationBitMask & kSystemApi) {
+        *out << prefix << "@android.annotation.SystemApi\n";
+    }
 }
 
 } // namespace aapt

tools/aapt2/java/AnnotationProcessor.h

@@ -19,6 +19,7 @@
 
 #include "util/StringPiece.h"
 
+#include <sstream>
 #include <string>
 
 namespace aapt {
@@ -53,13 +54,6 @@ namespace aapt {
  */
 class AnnotationProcessor {
 public:
-    /**
-     * Creates an AnnotationProcessor with a given prefix for each line generated.
-     * This is usually a set of spaces for indentation.
-     */
-    AnnotationProcessor(const StringPiece& prefix) : mPrefix(prefix.toString()) {
-    }
-
     /**
      * Adds more comments. Since resources can have various values with different configurations,
      * we need to collect all the comments.
@@ -68,23 +62,20 @@ public:
     void appendComment(const StringPiece& comment);
 
     /**
-     * Finishes the comment and moves it to the caller. Subsequent calls to buildComment() have
-     * undefined results.
+     * Writes the comments and annotations to the stream, with the given prefix before each line.
      */
-    std::string buildComment();
-
-    /**
-     * Finishes the annotation and moves it to the caller. Subsequent calls to buildAnnotations()
-     * have undefined results.
-     */
-    std::string buildAnnotations();
+    void writeToStream(std::ostream* out, const StringPiece& prefix);
 
 private:
-    std::string mPrefix;
-    std::string mComment;
-    std::string mAnnotations;
-    bool mDeprecated = false;
-    bool mSystemApi = false;
+    enum : uint32_t {
+        kDeprecated = 0x01,
+        kSystemApi = 0x02,
+    };
+
+    std::stringstream mComment;
+    std::stringstream mAnnotations;
+    bool mHasComments = false;
+    uint32_t mAnnotationBitMask = 0;
 
     void appendCommentLine(const std::string& line);
 };

tools/aapt2/java/AnnotationProcessor_test.cpp

@@ -0,0 +1,80 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "ResourceParser.h"
+#include "ResourceTable.h"
+#include "ResourceValues.h"
+#include "XmlPullParser.h"
+
+#include "java/AnnotationProcessor.h"
+
+#include "test/Builders.h"
+#include "test/Context.h"
+
+#include <gtest/gtest.h>
+
+namespace aapt {
+
+struct AnnotationProcessorTest : public ::testing::Test {
+    std::unique_ptr<IAaptContext> mContext;
+    ResourceTable mTable;
+
+    void SetUp() override {
+        mContext = test::ContextBuilder().build();
+    }
+
+    ::testing::AssertionResult parse(const StringPiece& str) {
+        ResourceParserOptions options;
+        ResourceParser parser(mContext->getDiagnostics(), &mTable, Source{}, ConfigDescription{},
+                              options);
+        std::stringstream in;
+        in << "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" << str;
+        XmlPullParser xmlParser(in);
+        if (parser.parse(&xmlParser)) {
+            return ::testing::AssertionSuccess();
+        }
+        return ::testing::AssertionFailure();
+    }
+};
+
+TEST_F(AnnotationProcessorTest, EmitsDeprecated) {
+    ASSERT_TRUE(parse(R"EOF(
+    <resources>
+      <declare-styleable name="foo">      
+        <!-- Some comment, and it should contain
+             a marker word, something that marks
+             this resource as nor needed.
+             {@deprecated That's the marker! } -->
+        <attr name="autoText" format="boolean" />
+      </declare-styleable>
+    </resources>)EOF"));
+
+    Attribute* attr = test::getValue<Attribute>(&mTable, u"@attr/autoText");
+    ASSERT_NE(nullptr, attr);
+
+    AnnotationProcessor processor;
+    processor.appendComment(attr->getComment());
+
+    std::stringstream result;
+    processor.writeToStream(&result, "");
+    std::string annotations = result.str();
+
+    EXPECT_NE(std::string::npos, annotations.find("@Deprecated"));
+}
+
+} // namespace aapt
+
+

tools/aapt2/java/ClassDefinitionWriter.h

@@ -0,0 +1,141 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef AAPT_JAVA_CLASSDEFINITION_H
+#define AAPT_JAVA_CLASSDEFINITION_H
+
+#include "java/AnnotationProcessor.h"
+#include "util/StringPiece.h"
+#include "util/Util.h"
+
+#include <sstream>
+#include <string>
+
+namespace aapt {
+
+struct ClassDefinitionWriterOptions {
+    bool useFinalQualifier = false;
+    bool forceCreationIfEmpty = false;
+};
+
+/**
+ * Writes a class for use in R.java or Manifest.java.
+ */
+class ClassDefinitionWriter {
+public:
+    ClassDefinitionWriter(const StringPiece& name, const ClassDefinitionWriterOptions& options) :
+            mName(name.toString()), mOptions(options), mStarted(false) {
+    }
+
+    ClassDefinitionWriter(const StringPiece16& name, const ClassDefinitionWriterOptions& options) :
+            mName(util::utf16ToUtf8(name)), mOptions(options), mStarted(false) {
+    }
+
+    void addIntMember(const StringPiece& name, AnnotationProcessor* processor,
+                      const uint32_t val) {
+        ensureClassDeclaration();
+        if (processor) {
+            processor->writeToStream(&mOut, kIndent);
+        }
+        mOut << kIndent << "public static " << (mOptions.useFinalQualifier ? "final " : "")
+             << "int " << name << "=" << val << ";\n";
+    }
+
+    void addStringMember(const StringPiece16& name, AnnotationProcessor* processor,
+                         const StringPiece16& val) {
+        ensureClassDeclaration();
+        if (processor) {
+            processor->writeToStream(&mOut, kIndent);
+        }
+        mOut << kIndent << "public static " << (mOptions.useFinalQualifier ? "final " : "")
+             << "String " << name << "=\"" << val << "\";\n";
+    }
+
+    void addResourceMember(const StringPiece16& name, AnnotationProcessor* processor,
+                           const ResourceId id) {
+        ensureClassDeclaration();
+        if (processor) {
+            processor->writeToStream(&mOut, kIndent);
+        }
+        mOut << kIndent << "public static " << (mOptions.useFinalQualifier ? "final " : "")
+             << "int " << name << "=" << id <<";\n";
+    }
+
+    template <typename Iterator, typename FieldAccessorFunc>
+    void addArrayMember(const StringPiece16& name, AnnotationProcessor* processor,
+                        const Iterator begin, const Iterator end, FieldAccessorFunc f) {
+        ensureClassDeclaration();
+        if (processor) {
+            processor->writeToStream(&mOut, kIndent);
+        }
+        mOut << kIndent << "public static final int[] " << name << "={";
+
+        for (Iterator current = begin; current != end; ++current) {
+            if (std::distance(begin, current) % kAttribsPerLine == 0) {
+                mOut << "\n" << kIndent << kIndent;
+            }
+
+            mOut << f(*current);
+            if (std::distance(current, end) > 1) {
+                mOut << ", ";
+            }
+        }
+        mOut << "\n" << kIndent <<"};\n";
+    }
+
+    void writeToStream(std::ostream* out, const StringPiece& prefix,
+                       AnnotationProcessor* processor=nullptr) {
+        if (mOptions.forceCreationIfEmpty) {
+            ensureClassDeclaration();
+        }
+
+        if (!mStarted) {
+            return;
+        }
+
+        if (processor) {
+            processor->writeToStream(out, prefix);
+        }
+
+        std::string result = mOut.str();
+        for (StringPiece line : util::tokenize<char>(result, '\n')) {
+            *out << prefix << line << "\n";
+        }
+        *out << prefix << "}\n";
+    }
+
+private:
+    constexpr static const char* kIndent = "  ";
+
+    // The number of attributes to emit per line in a Styleable array.
+    constexpr static size_t kAttribsPerLine = 4;
+
+    void ensureClassDeclaration() {
+        if (!mStarted) {
+            mStarted = true;
+            mOut << "public static final class " << mName << " {\n";
+        }
+    }
+
+    std::stringstream mOut;
+    std::string mName;
+    ClassDefinitionWriterOptions mOptions;
+    bool mStarted;
+};
+
+} // namespace aapt
+
+#endif /* AAPT_JAVA_CLASSDEFINITION_H */

tools/aapt2/java/JavaClassGenerator.cpp

@@ -21,7 +21,9 @@
 #include "ValueVisitor.h"
 
 #include "java/AnnotationProcessor.h"
+#include "java/ClassDefinitionWriter.h"
 #include "java/JavaClassGenerator.h"
+#include "util/Comparators.h"
 #include "util/StringPiece.h"
 
 #include <algorithm>
@@ -32,9 +34,6 @@
 
 namespace aapt {
 
-// The number of attributes to emit per line in a Styleable array.
-constexpr size_t kAttribsPerLine = 4;
-
 JavaClassGenerator::JavaClassGenerator(ResourceTable* table, JavaClassGeneratorOptions options) :
         mTable(table), mOptions(options) {
 }
@@ -92,12 +91,11 @@ bool JavaClassGenerator::skipSymbol(SymbolState state) {
     return true;
 }
 
-void JavaClassGenerator::generateStyleable(const StringPiece16& packageNameToGenerate,
-                                           const std::u16string& entryName,
-                                           const Styleable* styleable,
-                                           std::ostream* out) {
-    const StringPiece finalModifier = mOptions.useFinal ? " final" : "";
-
+void JavaClassGenerator::writeStyleableEntryForClass(ClassDefinitionWriter* outClassDef,
+                                                     AnnotationProcessor* processor,
+                                                     const StringPiece16& packageNameToGenerate,
+                                                     const std::u16string& entryName,
+                                                     const Styleable* styleable) {
     // This must be sorted by resource ID.
     std::vector<std::pair<ResourceId, ResourceNameRef>> sortedAttributes;
     sortedAttributes.reserve(styleable->entries.size());
@@ -110,36 +108,31 @@ void JavaClassGenerator::generateStyleable(const StringPiece16& packageNameToGen
     }
     std::sort(sortedAttributes.begin(), sortedAttributes.end());
 
+    auto accessorFunc = [](const std::pair<ResourceId, ResourceNameRef>& a) -> ResourceId {
+        return a.first;
+    };
+
     // First we emit the array containing the IDs of each attribute.
-    *out << "    "
-         << "public static final int[] " << transform(entryName) << " = {";
-
-    const size_t attrCount = sortedAttributes.size();
-    for (size_t i = 0; i < attrCount; i++) {
-        if (i % kAttribsPerLine == 0) {
-            *out << "\n      ";
-        }
-
-        *out << sortedAttributes[i].first;
-        if (i != attrCount - 1) {
-            *out << ", ";
-        }
-    }
-    *out << "\n    };\n";
+    outClassDef->addArrayMember(transform(entryName), processor,
+                                sortedAttributes.begin(),
+                                sortedAttributes.end(),
+                                accessorFunc);
 
     // Now we emit the indices into the array.
+    size_t attrCount = sortedAttributes.size();
     for (size_t i = 0; i < attrCount; i++) {
-        *out << "    "
-             << "public static" << finalModifier
-             << " int " << transform(entryName);
+        std::stringstream name;
+        name << transform(entryName);
 
         // We may reference IDs from other packages, so prefix the entry name with
         // the package.
         const ResourceNameRef& itemName = sortedAttributes[i].second;
         if (!itemName.package.empty() && packageNameToGenerate != itemName.package) {
-            *out << "_" << transform(itemName.package);
+            name << "_" << transform(itemName.package);
         }
-        *out << "_" << transform(itemName.entry) << " = " << i << ";\n";
+        name << "_" << transform(itemName.entry);
+
+        outClassDef->addIntMember(name.str(), nullptr, i);
     }
 }
 
@@ -155,8 +148,8 @@ static void addAttributeFormatDoc(AnnotationProcessor* processor, Attribute* att
 
     if (typeMask & android::ResTable_map::TYPE_STRING) {
         processor->appendComment(
-                "<p>May be a string value, using '\\;' to escape characters such as\n"
-                "'\\n' or '\\uxxxx' for a unicode character;");
+                "<p>May be a string value, using '\\\\;' to escape characters such as\n"
+                "'\\\\n' or '\\\\uxxxx' for a unicode character;");
     }
 
     if (typeMask & android::ResTable_map::TYPE_INTEGER) {
@@ -222,14 +215,10 @@ static void addAttributeFormatDoc(AnnotationProcessor* processor, Attribute* att
     }
 }
 
-bool JavaClassGenerator::generateType(const StringPiece16& packageNameToGenerate,
-                                      const ResourceTablePackage* package,
-                                      const ResourceTableType* type,
-                                      std::ostream* out) {
-    const StringPiece finalModifier = mOptions.useFinal ? " final" : "";
-
-    std::u16string unmangledPackage;
-    std::u16string unmangledName;
+bool JavaClassGenerator::writeEntriesForClass(ClassDefinitionWriter* outClassDef,
+                                              const StringPiece16& packageNameToGenerate,
+                                              const ResourceTablePackage* package,
+                                              const ResourceTableType* type) {
     for (const auto& entry : type->entries) {
         if (skipSymbol(entry->symbolStatus.state)) {
             continue;
@@ -238,7 +227,8 @@ bool JavaClassGenerator::generateType(const StringPiece16& packageNameToGenerate
         ResourceId id(package->id.value(), type->id.value(), entry->id.value());
         assert(id.isValid());
 
-        unmangledName = entry->name;
+        std::u16string unmangledPackage;
+        std::u16string unmangledName = entry->name;
         if (NameMangler::unmangle(&unmangledName, &unmangledPackage)) {
             // The entry name was mangled, and we successfully unmangled it.
             // Check that we want to emit this symbol.
@@ -246,12 +236,10 @@ bool JavaClassGenerator::generateType(const StringPiece16& packageNameToGenerate
                 // Skip the entry if it doesn't belong to the package we're writing.
                 continue;
             }
-        } else {
-            if (packageNameToGenerate != package->name) {
-                // We are processing a mangled package name,
-                // but this is a non-mangled resource.
-                continue;
-            }
+        } else if (packageNameToGenerate != package->name) {
+            // We are processing a mangled package name,
+            // but this is a non-mangled resource.
+            continue;
         }
 
         if (!isValidSymbol(unmangledName)) {
@@ -262,39 +250,33 @@ bool JavaClassGenerator::generateType(const StringPiece16& packageNameToGenerate
             return false;
         }
 
+        // Build the comments and annotations for this entry.
+
+        AnnotationProcessor processor;
+        if (entry->symbolStatus.state != SymbolState::kUndefined) {
+            processor.appendComment(entry->symbolStatus.comment);
+        }
+
+        for (const auto& configValue : entry->values) {
+            processor.appendComment(configValue.value->getComment());
+        }
+
+        // If this is an Attribute, append the format Javadoc.
+        if (!entry->values.empty()) {
+            if (Attribute* attr = valueCast<Attribute>(entry->values.front().value.get())) {
+                // We list out the available values for the given attribute.
+                addAttributeFormatDoc(&processor, attr);
+            }
+        }
+
         if (type->type == ResourceType::kStyleable) {
             assert(!entry->values.empty());
-            generateStyleable(packageNameToGenerate, unmangledName, static_cast<const Styleable*>(
-                    entry->values.front().value.get()), out);
+            const Styleable* styleable = static_cast<const Styleable*>(
+                    entry->values.front().value.get());
+            writeStyleableEntryForClass(outClassDef, &processor, packageNameToGenerate,
+                                        unmangledName, styleable);
         } else {
-            AnnotationProcessor processor("    ");
-            if (entry->symbolStatus.state != SymbolState::kUndefined) {
-                processor.appendComment(entry->symbolStatus.comment);
-            }
-
-            for (const auto& configValue : entry->values) {
-                processor.appendComment(configValue.value->getComment());
-            }
-
-            if (!entry->values.empty()) {
-                if (Attribute* attr = valueCast<Attribute>(entry->values.front().value.get())) {
-                    // We list out the available values for the given attribute.
-                    addAttributeFormatDoc(&processor, attr);
-                }
-            }
-
-            std::string comment = processor.buildComment();
-            if (!comment.empty()) {
-                *out << comment << "\n";
-            }
-
-            std::string annotations = processor.buildAnnotations();
-            if (!annotations.empty()) {
-                *out << annotations << "\n";
-            }
-
-            *out << "    " << "public static" << finalModifier
-                 << " int " << transform(unmangledName) << " = " << id << ";\n";
+            outClassDef->addResourceMember(transform(unmangledName), &processor, id);
         }
     }
     return true;
@@ -312,17 +294,42 @@ bool JavaClassGenerator::generate(const StringPiece16& packageNameToGenerate,
 
     for (const auto& package : mTable->packages) {
         for (const auto& type : package->types) {
-            StringPiece16 typeStr;
             if (type->type == ResourceType::kAttrPrivate) {
-                typeStr = toString(ResourceType::kAttr);
-            } else {
-                typeStr = toString(type->type);
+                continue;
             }
-            *out << "  public static final class " << typeStr << " {\n";
-            if (!generateType(packageNameToGenerate, package.get(), type.get(), out)) {
+
+            ClassDefinitionWriterOptions classOptions;
+            classOptions.useFinalQualifier = mOptions.useFinal;
+            classOptions.forceCreationIfEmpty =
+                    (mOptions.types == JavaClassGeneratorOptions::SymbolTypes::kPublic);
+            ClassDefinitionWriter classDef(toString(type->type), classOptions);
+            bool result = writeEntriesForClass(&classDef, packageNameToGenerate,
+                                               package.get(), type.get());
+            if (!result) {
                 return false;
             }
-            *out << "  }\n";
+
+            if (type->type == ResourceType::kAttr) {
+                // Also include private attributes in this same class.
+                auto iter = std::lower_bound(package->types.begin(), package->types.end(),
+                                             ResourceType::kAttrPrivate, cmp::lessThanType);
+                if (iter != package->types.end() && (*iter)->type == ResourceType::kAttrPrivate) {
+                    result = writeEntriesForClass(&classDef, packageNameToGenerate,
+                                                  package.get(), iter->get());
+                    if (!result) {
+                        return false;
+                    }
+                }
+            }
+
+            AnnotationProcessor processor;
+            if (type->type == ResourceType::kStyleable &&
+                    mOptions.types == JavaClassGeneratorOptions::SymbolTypes::kPublic) {
+                // When generating a public R class, we don't want Styleable to be part of the API.
+                // It is only emitted for documentation purposes.
+                processor.appendComment("@doconly");
+            }
+            classDef.writeToStream(out, "  ", &processor);
         }
     }
 

tools/aapt2/java/JavaClassGenerator.h

@@ -27,6 +27,9 @@
 
 namespace aapt {
 
+class AnnotationProcessor;
+class ClassDefinitionWriter;
+
 struct JavaClassGeneratorOptions {
     /*
      * Specifies whether to use the 'final' modifier
@@ -40,9 +43,6 @@ struct JavaClassGeneratorOptions {
         kPublic,
     };
 
-    /*
-     *
-     */
     SymbolTypes types = SymbolTypes::kAll;
 };
 
@@ -69,15 +69,16 @@ public:
     const std::string& getError() const;
 
 private:
-    bool generateType(const StringPiece16& packageNameToGenerate,
-                      const ResourceTablePackage* package,
-                      const ResourceTableType* type,
-                      std::ostream* out);
+    bool writeEntriesForClass(ClassDefinitionWriter* outClassDef,
+                              const StringPiece16& packageNameToGenerate,
+                              const ResourceTablePackage* package,
+                              const ResourceTableType* type);
 
-    void generateStyleable(const StringPiece16& packageNameToGenerate,
-                           const std::u16string& entryName,
-                           const Styleable* styleable,
-                           std::ostream* out);
+    void writeStyleableEntryForClass(ClassDefinitionWriter* outClassDef,
+                                     AnnotationProcessor* processor,
+                                     const StringPiece16& packageNameToGenerate,
+                                     const std::u16string& entryName,
+                                     const Styleable* styleable);
 
     bool skipSymbol(SymbolState state);
 

tools/aapt2/java/JavaClassGenerator_test.cpp

@@ -56,13 +56,13 @@ TEST(JavaClassGeneratorTest, TransformInvalidJavaIdentifierCharacter) {
     std::string output = out.str();
 
     EXPECT_NE(std::string::npos,
-              output.find("public static final int hey_man = 0x01020000;"));
+              output.find("public static final int hey_man=0x01020000;"));
 
     EXPECT_NE(std::string::npos,
-              output.find("public static final int[] hey_dude = {"));
+              output.find("public static final int[] hey_dude={"));
 
     EXPECT_NE(std::string::npos,
-              output.find("public static final int hey_dude_cool_attr = 0;"));
+              output.find("public static final int hey_dude_cool_attr=0;"));
 }
 
 TEST(JavaClassGeneratorTest, CorrectPackageNameIsUsed) {
@@ -78,7 +78,7 @@ TEST(JavaClassGeneratorTest, CorrectPackageNameIsUsed) {
 
     std::string output = out.str();
     EXPECT_NE(std::string::npos, output.find("package com.android.internal;"));
-    EXPECT_NE(std::string::npos, output.find("public static final int one = 0x01020000;"));
+    EXPECT_NE(std::string::npos, output.find("public static final int one=0x01020000;"));
     EXPECT_EQ(std::string::npos, output.find("two"));
     EXPECT_EQ(std::string::npos, output.find("com_foo$two"));
 }
@@ -86,6 +86,7 @@ TEST(JavaClassGeneratorTest, CorrectPackageNameIsUsed) {
 TEST(JavaClassGeneratorTest, AttrPrivateIsWrittenAsAttr) {
     std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder()
             .setPackageId(u"android", 0x01)
+            .addSimple(u"@android:attr/two", ResourceId(0x01010001))
             .addSimple(u"@android:^attr-private/one", ResourceId(0x01010000))
             .build();
 
@@ -116,7 +117,7 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
         std::stringstream out;
         ASSERT_TRUE(generator.generate(u"android", &out));
         std::string output = out.str();
-        EXPECT_NE(std::string::npos, output.find("public static final int one = 0x01020000;"));
+        EXPECT_NE(std::string::npos, output.find("public static final int one=0x01020000;"));
         EXPECT_EQ(std::string::npos, output.find("two"));
         EXPECT_EQ(std::string::npos, output.find("three"));
     }
@@ -127,8 +128,8 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
         std::stringstream out;
         ASSERT_TRUE(generator.generate(u"android", &out));
         std::string output = out.str();
-        EXPECT_NE(std::string::npos, output.find("public static final int one = 0x01020000;"));
-        EXPECT_NE(std::string::npos, output.find("public static final int two = 0x01020001;"));
+        EXPECT_NE(std::string::npos, output.find("public static final int one=0x01020000;"));
+        EXPECT_NE(std::string::npos, output.find("public static final int two=0x01020001;"));
         EXPECT_EQ(std::string::npos, output.find("three"));
     }
 
@@ -138,9 +139,9 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
         std::stringstream out;
         ASSERT_TRUE(generator.generate(u"android", &out));
         std::string output = out.str();
-        EXPECT_NE(std::string::npos, output.find("public static final int one = 0x01020000;"));
-        EXPECT_NE(std::string::npos, output.find("public static final int two = 0x01020001;"));
-        EXPECT_NE(std::string::npos, output.find("public static final int three = 0x01020002;"));
+        EXPECT_NE(std::string::npos, output.find("public static final int one=0x01020000;"));
+        EXPECT_NE(std::string::npos, output.find("public static final int two=0x01020001;"));
+        EXPECT_NE(std::string::npos, output.find("public static final int three=0x01020002;"));
     }
 }
 
@@ -194,8 +195,8 @@ TEST(JavaClassGeneratorTest, EmitOtherPackagesAttributesInStyleable) {
     EXPECT_TRUE(generator.generate(u"android", &out));
 
     std::string output = out.str();
-    EXPECT_NE(std::string::npos, output.find("int foo_bar ="));
-    EXPECT_NE(std::string::npos, output.find("int foo_com_lib_bar ="));
+    EXPECT_NE(std::string::npos, output.find("int foo_bar="));
+    EXPECT_NE(std::string::npos, output.find("int foo_com_lib_bar="));
 }
 
 TEST(JavaClassGeneratorTest, CommentsForSimpleResourcesArePresent) {
@@ -218,7 +219,7 @@ TEST(JavaClassGeneratorTest, CommentsForSimpleResourcesArePresent) {
      * @deprecated
      */
     @Deprecated
-    public static final int foo = 0x01010000;)EOF"));
+    public static final int foo=0x01010000;)EOF"));
 }
 
 TEST(JavaClassGeneratorTest, CommentsForEnumAndFlagAttributesArePresent) {

tools/aapt2/java/ManifestClassGenerator.cpp

@@ -18,6 +18,7 @@
 #include "XmlDom.h"
 
 #include "java/AnnotationProcessor.h"
+#include "java/ClassDefinitionWriter.h"
 #include "java/ManifestClassGenerator.h"
 #include "util/Maybe.h"
 
@@ -58,8 +59,8 @@ static Maybe<StringPiece16> extractJavaIdentifier(IDiagnostics* diag, const Sour
     return result;
 }
 
-static bool writeSymbol(IDiagnostics* diag, const Source& source, xml::Element* el,
-                        std::ostream* out) {
+static bool writeSymbol(IDiagnostics* diag, ClassDefinitionWriter* outClassDef, const Source& source,
+                        xml::Element* el) {
     xml::Attribute* attr = el->findAttribute(xml::kSchemaAndroid, u"name");
     if (!attr) {
         diag->error(DiagMessage(source) << "<" << el->name << "> must define 'android:name'");
@@ -72,18 +73,9 @@ static bool writeSymbol(IDiagnostics* diag, const Source& source, xml::Element*
         return false;
     }
 
-    *out << "\n";
-
-    if (!util::trimWhitespace(el->comment).empty()) {
-        AnnotationProcessor processor("    ");
-        processor.appendComment(el->comment);
-        *out << processor.buildComment() << "\n";
-        std::string annotations = processor.buildAnnotations();
-        if (!annotations.empty()) {
-            *out << annotations << "\n";
-        }
-    }
-    *out << "    public static final String " << result.value() << "=\"" << attr->value << "\";\n";
+    AnnotationProcessor processor;
+    processor.appendComment(el->comment);
+    outClassDef->addStringMember(result.value(), &processor, attr->value);
     return true;
 }
 
@@ -100,29 +92,32 @@ bool ManifestClassGenerator::generate(IDiagnostics* diag, const StringPiece16& p
     }
 
     *out << "package " << package << ";\n\n"
-         << "public class Manifest {\n";
+         << "public final class Manifest {\n";
 
     bool error = false;
     std::vector<xml::Element*> children = el->getChildElements();
 
+    ClassDefinitionWriterOptions classOptions;
+    classOptions.useFinalQualifier = true;
+    classOptions.forceCreationIfEmpty = false;
 
     // First write out permissions.
-    *out << "  public static class permission {\n";
+    ClassDefinitionWriter classDef("permission", classOptions);
     for (xml::Element* childEl : children) {
         if (childEl->namespaceUri.empty() && childEl->name == u"permission") {
-            error |= !writeSymbol(diag, res->file.source, childEl, out);
+            error |= !writeSymbol(diag, &classDef, res->file.source, childEl);
         }
     }
-    *out << "  }\n";
+    classDef.writeToStream(out, "  ");
 
     // Next write out permission groups.
-    *out << "  public static class permission_group {\n";
+    classDef = ClassDefinitionWriter("permission_group", classOptions);
     for (xml::Element* childEl : children) {
         if (childEl->namespaceUri.empty() && childEl->name == u"permission-group") {
-            error |= !writeSymbol(diag, res->file.source, childEl, out);
+            error |= !writeSymbol(diag, &classDef, res->file.source, childEl);
         }
     }
-    *out << "  }\n";
+    classDef.writeToStream(out, "  ");
 
     *out << "}\n";
     return !error;

tools/aapt2/java/ManifestClassGenerator_test.cpp

@@ -39,8 +39,9 @@ TEST(ManifestClassGeneratorTest, NameIsProperlyGeneratedFromSymbol) {
 
     std::string actual = out.str();
 
-    const size_t permissionClassPos = actual.find("public static class permission {");
-    const size_t permissionGroupClassPos = actual.find("public static class permission_group {");
+    const size_t permissionClassPos = actual.find("public static final class permission {");
+    const size_t permissionGroupClassPos =
+            actual.find("public static final class permission_group {");
     ASSERT_NE(std::string::npos, permissionClassPos);
     ASSERT_NE(std::string::npos, permissionGroupClassPos);
 
@@ -113,7 +114,7 @@ R"EOF(    /**
      * @hide
      * @SystemApi
      */
-    @android.annotations.SystemApi
+    @android.annotation.SystemApi
     public static final String SECRET="android.permission.SECRET";)EOF"));
 }
 

tools/aapt2/link/AutoVersioner.cpp

@@ -31,7 +31,7 @@ bool shouldGenerateVersionedResource(const ResourceEntry* entry, const ConfigDes
                                      const int sdkVersionToGenerate) {
     assert(sdkVersionToGenerate > config.sdkVersion);
     const auto endIter = entry->values.end();
-    auto iter = std::lower_bound(entry->values.begin(), endIter, config, cmp::lessThan);
+    auto iter = std::lower_bound(entry->values.begin(), endIter, config, cmp::lessThanConfig);
 
     // The source config came from this list, so it should be here.
     assert(iter != entry->values.end());
@@ -124,7 +124,7 @@ bool AutoVersioner::consume(IAaptContext* context, ResourceTable* table) {
                                 auto iter = std::lower_bound(entry->values.begin(),
                                                              entry->values.end(),
                                                              newConfig,
-                                                             cmp::lessThan);
+                                                             cmp::lessThanConfig);
 
                                 entry->values.insert(
                                         iter,

tools/aapt2/link/TableMerger.cpp

@@ -122,7 +122,7 @@ bool TableMerger::doMerge(const Source& src, ResourceTable* srcTable,
 
             for (ResourceConfigValue& srcValue : srcEntry->values) {
                 auto iter = std::lower_bound(dstEntry->values.begin(), dstEntry->values.end(),
-                                             srcValue.config, cmp::lessThan);
+                                             srcValue.config, cmp::lessThanConfig);
 
                 if (iter != dstEntry->values.end() && iter->config == srcValue.config) {
                     const int collisionResult = ResourceTable::resolveValueCollision(

tools/aapt2/process/SymbolTable.cpp

@@ -55,7 +55,7 @@ const ISymbolTable::Symbol* SymbolTableWrapper::findByName(const ResourceName& n
     if (name.type == ResourceType::kAttr || name.type == ResourceType::kAttrPrivate) {
         const ConfigDescription kDefaultConfig;
         auto iter = std::lower_bound(sr.entry->values.begin(), sr.entry->values.end(),
-                                     kDefaultConfig, cmp::lessThan);
+                                     kDefaultConfig, cmp::lessThanConfig);
 
         if (iter != sr.entry->values.end() && iter->config == kDefaultConfig) {
             // This resource has an Attribute.

tools/aapt2/util/Comparators.h

@@ -17,13 +17,20 @@
 #ifndef AAPT_UTIL_COMPARATORS_H
 #define AAPT_UTIL_COMPARATORS_H
 
+#include "ConfigDescription.h"
+#include "ResourceTable.h"
+
 namespace aapt {
 namespace cmp {
 
-inline bool lessThan(const ResourceConfigValue& a, const ConfigDescription& b) {
+inline bool lessThanConfig(const ResourceConfigValue& a, const ConfigDescription& b) {
     return a.config < b;
 }
 
+inline bool lessThanType(const std::unique_ptr<ResourceTableType>& a, ResourceType b) {
+    return a->type < b;
+}
+
 } // namespace cmp
 } // namespace aapt