Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e78fd617 authored by Adam Lesinski's avatar Adam Lesinski
Browse files

AAPT2: Move comments and source into Value 

Values are closely related to where they were defined, so
this information should live inside the Value.

This also enables comments to be attached to nested Values.

Change-Id: Ic7481b5a5f26d0ef248d638e2e29252f88154581
parent 3b7acbb8

tools/aapt2/JavaClassGenerator_test.cpp

+2 −4
Original line number Diff line number Diff line
@@ -105,11 +105,9 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) { 
            .addSimple(u"@android:id/one", ResourceId(0x01020000))

            .addSimple(u"@android:id/two", ResourceId(0x01020001))

            .addSimple(u"@android:id/three", ResourceId(0x01020002))

            .setSymbolState(u"@android:id/one", ResourceId(0x01020000), SymbolState::kPublic)

            .setSymbolState(u"@android:id/two", ResourceId(0x01020001), SymbolState::kPrivate)

            .build();

    ASSERT_TRUE(table->setSymbolState(test::parseNameOrDie(u"@android:id/one"), {}, {},

                                      SymbolState::kPublic, &diag));

    ASSERT_TRUE(table->setSymbolState(test::parseNameOrDie(u"@android:id/two"), {}, {},

                                      SymbolState::kPrivate, &diag));



    JavaClassGeneratorOptions options;

    options.types = JavaClassGeneratorOptions::SymbolTypes::kPublic;

tools/aapt2/ResourceParser.cpp

+21 −9
Original line number Diff line number Diff line
@@ -186,6 +186,7 @@ struct ParsedResource { 
    Source source;

    ResourceId id;

    SymbolState symbolState = SymbolState::kUndefined;

    std::u16string comment;

    std::unique_ptr<Value> value;

    std::list<ParsedResource> childResources;

};
@@ -194,7 +195,11 @@ struct ParsedResource { 
static bool addResourcesToTable(ResourceTable* table, const ConfigDescription& config,

                                IDiagnostics* diag, ParsedResource* res) {

    if (res->symbolState != SymbolState::kUndefined) {

        if (!table->setSymbolState(res->name, res->id, res->source, res->symbolState, diag)) {

        Symbol symbol;

        symbol.state = res->symbolState;

        symbol.source = res->source;

        symbol.comment = res->comment;

        if (!table->setSymbolState(res->name, res->id, symbol, diag)) {

            return false;

        }

    }
@@ -203,7 +208,11 @@ static bool addResourcesToTable(ResourceTable* table, const ConfigDescription& c 
        return true;

    }



    if (!table->addResource(res->name, res->id, config, res->source, std::move(res->value), diag)) {

    // Attach the comment, source and config to the value.

    res->value->setComment(std::move(res->comment));

    res->value->setSource(std::move(res->source));



    if (!table->addResource(res->name, res->id, config, std::move(res->value), diag)) {

        return false;

    }
@@ -275,6 +284,7 @@ bool ResourceParser::parseResources(XmlPullParser* parser) { 
        ParsedResource parsedResource;

        parsedResource.name.entry = maybeName.value().toString();

        parsedResource.source = mSource.withLine(parser->getLineNumber());

        parsedResource.comment = std::move(comment);



        bool result = true;

        if (elementName == u"id") {
@@ -368,8 +378,8 @@ enum { 
 * an Item. If allowRawValue is false, nullptr is returned in this

 * case.

 */

std::unique_ptr<Item> ResourceParser::parseXml(XmlPullParser* parser, uint32_t typeMask,

                                               bool allowRawValue) {

std::unique_ptr<Item> ResourceParser::parseXml(XmlPullParser* parser, const uint32_t typeMask,

                                               const bool allowRawValue) {

    const size_t beginXmlLine = parser->getLineNumber();



    std::u16string rawValue;
@@ -386,8 +396,9 @@ std::unique_ptr<Item> ResourceParser::parseXml(XmlPullParser* parser, uint32_t t 


    auto onCreateReference = [&](const ResourceName& name) {

        // name.package can be empty here, as it will assume the package name of the table.

        mTable->addResource(name, {}, mSource.withLine(beginXmlLine), util::make_unique<Id>(),

                            mDiag);

        std::unique_ptr<Id> id = util::make_unique<Id>();

        id->setSource(mSource.withLine(beginXmlLine));

        mTable->addResource(name, {}, std::move(id), mDiag);

    };



    // Process the raw value.
@@ -411,11 +422,12 @@ std::unique_ptr<Item> ResourceParser::parseXml(XmlPullParser* parser, uint32_t t 
                mTable->stringPool.makeRef(styleString.str, StringPool::Context{ 1, mConfig }));

    }



    // We can't parse this so return a RawString if we are allowed.

    if (allowRawValue) {

        // We can't parse this so return a RawString if we are allowed.

        return util::make_unique<RawString>(

                mTable->stringPool.makeRef(rawValue, StringPool::Context{ 1, mConfig }));

    }



    return {};

}
@@ -683,7 +695,7 @@ Maybe<Attribute::Symbol> ResourceParser::parseEnumOrFlagItem(XmlPullParser* pars 
    }



    return Attribute::Symbol{

        Reference(ResourceName{ {}, ResourceType::kId, maybeName.value().toString() }),

            Reference(ResourceName({}, ResourceType::kId, maybeName.value().toString())),

            val.data };

}

tools/aapt2/ResourceParser.h

+6 −5
Original line number Diff line number Diff line
@@ -65,12 +65,13 @@ private: 
                           StyleString* outStyleString);



    /*

     * Parses the XML subtree and converts it to an Item. The type of Item that can be

     * parsed is denoted by the `typeMask`. If `allowRawValue` is true and the subtree

     * can not be parsed as a regular Item, then a RawString is returned. Otherwise

     * this returns nullptr.

     * Parses the XML subtree and returns an Item.

     * The type of Item that can be parsed is denoted by the `typeMask`.

     * If `allowRawValue` is true and the subtree can not be parsed as a regular Item, then a

     * RawString is returned. Otherwise this returns false;

     */

    std::unique_ptr<Item> parseXml(XmlPullParser* parser, uint32_t typeMask, bool allowRawValue);

    std::unique_ptr<Item> parseXml(XmlPullParser* parser, const uint32_t typeMask,

                                   const bool allowRawValue);



    bool parseResources(XmlPullParser* parser);

    bool parseString(XmlPullParser* parser, ParsedResource* outResource);

tools/aapt2/ResourceParser_test.cpp

+29 −8
Original line number Diff line number Diff line
@@ -383,14 +383,35 @@ TEST_F(ResourceParserTest, ParseCommentsWithResource) { 
                        "<string name=\"foo\">Hi</string>";

    ASSERT_TRUE(testParse(input));



    Maybe<ResourceTable::SearchResult> result = mTable.findResource(

            test::parseNameOrDie(u"@string/foo"));

    AAPT_ASSERT_TRUE(result);



    ResourceEntry* entry = result.value().entry;

    ASSERT_NE(entry, nullptr);

    ASSERT_FALSE(entry->values.empty());

    EXPECT_EQ(entry->values.front().comment, u"This is a comment");

    String* value = test::getValue<String>(&mTable, u"@string/foo");

    ASSERT_NE(nullptr, value);

    EXPECT_EQ(value->getComment(), u"This is a comment");

}



TEST_F(ResourceParserTest, DoNotCombineMultipleComments) {

    std::string input = "<!--One-->\n"

                        "<!--Two-->\n"

                        "<string name=\"foo\">Hi</string>";



    ASSERT_TRUE(testParse(input));



    String* value = test::getValue<String>(&mTable, u"@string/foo");

    ASSERT_NE(nullptr, value);

    EXPECT_EQ(value->getComment(), u"Two");

}



TEST_F(ResourceParserTest, IgnoreCommentBeforeEndTag) {

    std::string input = "<!--One-->\n"

                        "<string name=\"foo\">\n"

                        "  Hi\n"

                        "<!--Two-->\n"

                        "</string>";



    ASSERT_TRUE(testParse(input));



    String* value = test::getValue<String>(&mTable, u"@string/foo");

    ASSERT_NE(nullptr, value);

    EXPECT_EQ(value->getComment(), u"One");

}



/*

tools/aapt2/ResourceTable.cpp

+47 −50
Original line number Diff line number Diff line
@@ -19,6 +19,8 @@ 
#include "ResourceTable.h"

#include "ResourceValues.h"

#include "ValueVisitor.h"



#include "util/Comparators.h"

#include "util/Util.h"



#include <algorithm>
@@ -29,10 +31,6 @@ 


namespace aapt {



static bool compareConfigs(const ResourceConfigValue& lhs, const ConfigDescription& rhs) {

    return lhs.config < rhs;

}



static bool lessThanType(const std::unique_ptr<ResourceTableType>& lhs, ResourceType rhs) {

    return lhs->type < rhs;

}
@@ -191,52 +189,50 @@ static constexpr const char16_t* kValidNameChars = u"._-"; 
static constexpr const char16_t* kValidNameMangledChars = u"._-$";



bool ResourceTable::addResource(const ResourceNameRef& name, const ConfigDescription& config,

                                const Source& source, std::unique_ptr<Value> value,

                                IDiagnostics* diag) {

    return addResourceImpl(name, ResourceId{}, config, source, std::move(value), kValidNameChars,

                           diag);

                                std::unique_ptr<Value> value, IDiagnostics* diag) {

    return addResourceImpl(name, {}, config, std::move(value), kValidNameChars, diag);

}



bool ResourceTable::addResource(const ResourceNameRef& name, const ResourceId resId,

                                const ConfigDescription& config, const Source& source,

                                std::unique_ptr<Value> value, IDiagnostics* diag) {

    return addResourceImpl(name, resId, config, source, std::move(value), kValidNameChars, diag);

                                const ConfigDescription& config, std::unique_ptr<Value> value,

                                IDiagnostics* diag) {

    return addResourceImpl(name, resId, config, std::move(value), kValidNameChars, diag);

}



bool ResourceTable::addFileReference(const ResourceNameRef& name, const ConfigDescription& config,

                                     const Source& source, const StringPiece16& path,

                                     IDiagnostics* diag) {

    return addResourceImpl(name, ResourceId{}, config, source,

                           util::make_unique<FileReference>(stringPool.makeRef(path)),

                           kValidNameChars, diag);

    std::unique_ptr<FileReference> fileRef = util::make_unique<FileReference>(

            stringPool.makeRef(path));

    fileRef->setSource(source);

    return addResourceImpl(name, ResourceId{}, config, std::move(fileRef), kValidNameChars, diag);

}



bool ResourceTable::addResourceAllowMangled(const ResourceNameRef& name,

                                            const ConfigDescription& config,

                                            const Source& source,

                                            std::unique_ptr<Value> value,

                                            IDiagnostics* diag) {

    return addResourceImpl(name, ResourceId{}, config, source, std::move(value),

                           kValidNameMangledChars, diag);

    return addResourceImpl(name, ResourceId{}, config, std::move(value), kValidNameMangledChars,

                           diag);

}



bool ResourceTable::addResourceAllowMangled(const ResourceNameRef& name,

                                            const ResourceId id,

                                            const ConfigDescription& config,

                                            const Source& source,

                                            std::unique_ptr<Value> value,

                                            IDiagnostics* diag) {

    return addResourceImpl(name, id, config, source, std::move(value),

                           kValidNameMangledChars, diag);

    return addResourceImpl(name, id, config, std::move(value), kValidNameMangledChars, diag);

}



bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceId resId,

                                    const ConfigDescription& config, const Source& source,

                                    std::unique_ptr<Value> value, const char16_t* validChars,

                                    IDiagnostics* diag) {

                                    const ConfigDescription& config, std::unique_ptr<Value> value,

                                    const char16_t* validChars, IDiagnostics* diag) {

    assert(value && "value can't be nullptr");

    assert(diag && "diagnostics can't be nullptr");



    auto badCharIter = util::findNonAlphaNumericAndNotInSet(name.entry, validChars);

    if (badCharIter != name.entry.end()) {

        diag->error(DiagMessage(source)

        diag->error(DiagMessage(value->getSource())

                    << "resource '"

                    << name

                    << "' has invalid entry name '"
@@ -249,7 +245,7 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI 


    ResourceTablePackage* package = findOrCreatePackage(name.package);

    if (resId.isValid() && package->id && package->id.value() != resId.packageId()) {

        diag->error(DiagMessage(source)

        diag->error(DiagMessage(value->getSource())

                    << "trying to add resource '"

                    << name

                    << "' with ID "
@@ -263,7 +259,7 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI 


    ResourceTableType* type = package->findOrCreateType(name.type);

    if (resId.isValid() && type->id && type->id.value() != resId.typeId()) {

        diag->error(DiagMessage(source)

        diag->error(DiagMessage(value->getSource())

                    << "trying to add resource '"

                    << name

                    << "' with ID "
@@ -277,7 +273,7 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI 


    ResourceEntry* entry = type->findOrCreateEntry(name.entry);

    if (resId.isValid() && entry->id && entry->id.value() != resId.entryId()) {

        diag->error(DiagMessage(source)

        diag->error(DiagMessage(value->getSource())

                    << "trying to add resource '"

                    << name

                    << "' with ID "
@@ -288,20 +284,20 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI 
    }



    const auto endIter = entry->values.end();

    auto iter = std::lower_bound(entry->values.begin(), endIter, config, compareConfigs);

    auto iter = std::lower_bound(entry->values.begin(), endIter, config, cmp::lessThan);

    if (iter == endIter || iter->config != config) {

        // This resource did not exist before, add it.

        entry->values.insert(iter, ResourceConfigValue{ config, source, {}, std::move(value) });

        entry->values.insert(iter, ResourceConfigValue{ config, std::move(value) });

    } else {

        int collisionResult = resolveValueCollision(iter->value.get(), value.get());

        if (collisionResult > 0) {

            // Take the incoming value.

            *iter = ResourceConfigValue{ config, source, {}, std::move(value) };

            iter->value = std::move(value);

        } else if (collisionResult == 0) {

            diag->error(DiagMessage(source)

            diag->error(DiagMessage(value->getSource())

                        << "duplicate value for resource '" << name << "' "

                        << "with config '" << iter->config << "'");

            diag->error(DiagMessage(iter->source)

                        << "with config '" << config << "'");

            diag->error(DiagMessage(iter->value->getSource())

                        << "resource previously defined here");

            return false;

        }
@@ -316,27 +312,29 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI 
}



bool ResourceTable::setSymbolState(const ResourceNameRef& name, const ResourceId resId,

                                   const Source& source, SymbolState state, IDiagnostics* diag) {

    return setSymbolStateImpl(name, resId, source, state, kValidNameChars, diag);

                                   const Symbol& symbol, IDiagnostics* diag) {

    return setSymbolStateImpl(name, resId, symbol, kValidNameChars, diag);

}



bool ResourceTable::setSymbolStateAllowMangled(const ResourceNameRef& name, const ResourceId resId,

                                               const Source& source, SymbolState state,

                                               IDiagnostics* diag) {

    return setSymbolStateImpl(name, resId, source, state, kValidNameMangledChars, diag);

bool ResourceTable::setSymbolStateAllowMangled(const ResourceNameRef& name,

                                               const ResourceId resId,

                                               const Symbol& symbol, IDiagnostics* diag) {

    return setSymbolStateImpl(name, resId, symbol, kValidNameMangledChars, diag);

}



bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const ResourceId resId,

                                       const Source& source, SymbolState state,

                                       const char16_t* validChars, IDiagnostics* diag) {

    if (state == SymbolState::kUndefined) {

                                       const Symbol& symbol, const char16_t* validChars,

                                       IDiagnostics* diag) {

    assert(diag && "diagnostics can't be nullptr");



    if (symbol.state == SymbolState::kUndefined) {

        // Nothing to do.

        return true;

    }



    auto badCharIter = util::findNonAlphaNumericAndNotInSet(name.entry, validChars);

    if (badCharIter != name.entry.end()) {

        diag->error(DiagMessage(source)

        diag->error(DiagMessage(symbol.source)

                    << "resource '"

                    << name

                    << "' has invalid entry name '"
@@ -349,7 +347,7 @@ bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const Resour 


    ResourceTablePackage* package = findOrCreatePackage(name.package);

    if (resId.isValid() && package->id && package->id.value() != resId.packageId()) {

        diag->error(DiagMessage(source)

        diag->error(DiagMessage(symbol.source)

                    << "trying to add resource '"

                    << name

                    << "' with ID "
@@ -363,7 +361,7 @@ bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const Resour 


    ResourceTableType* type = package->findOrCreateType(name.type);

    if (resId.isValid() && type->id && type->id.value() != resId.typeId()) {

        diag->error(DiagMessage(source)

        diag->error(DiagMessage(symbol.source)

                    << "trying to add resource '"

                    << name

                    << "' with ID "
@@ -377,7 +375,7 @@ bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const Resour 


    ResourceEntry* entry = type->findOrCreateEntry(name.entry);

    if (resId.isValid() && entry->id && entry->id.value() != resId.entryId()) {

        diag->error(DiagMessage(source)

        diag->error(DiagMessage(symbol.source)

                    << "trying to add resource '"

                    << name

                    << "' with ID "
@@ -388,15 +386,14 @@ bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const Resour 
    }



    // Only mark the type state as public, it doesn't care about being private.

    if (state == SymbolState::kPublic) {

    if (symbol.state == SymbolState::kPublic) {

        type->symbolStatus.state = SymbolState::kPublic;

    }



    // Downgrading to a private symbol from a public one is not allowed.

    if (entry->symbolStatus.state != SymbolState::kPublic) {

        if (entry->symbolStatus.state != state) {

            entry->symbolStatus.state = state;

            entry->symbolStatus.source = source;

        if (entry->symbolStatus.state != symbol.state) {

            entry->symbolStatus = std::move(symbol);

        }

    }