Improve the Parser's API and Storage of Metadata #3003

InsertCreativityHere · 2024-10-29T17:28:24Z

This PR improves the infrastructure and API around metadata.

The main change is that metadata is no longer stored directly as a string.
Now it has its own Metadata grammar type, which stores a pre-parsed version of the metadata.
Having the metadata pre-parsed into directive[:arguments] lets us offer a better API over it.

The first file in this PR (Grammar.cpp) is the generated parser. You don't need to review it.

InsertCreativityHere · 2024-10-29T18:02:24Z

cpp/src/Slice/Parser.h

+    // ----------------------------------------------------------------------
+    // GrammarBase
+    // ----------------------------------------------------------------------
+
+    class GrammarBase
+    {
+    public:
+        virtual ~GrammarBase() = default;
+    };


This was just moved up from lower in the file. This definition has to come before Metadata, and Metadata has to come before some other things which also use it.

InsertCreativityHere · 2024-10-29T18:02:47Z

cpp/src/slice2cs/CsUtil.cpp

@@ -1142,7 +1127,7 @@ Slice::CsGenerator::writeSequenceMarshalUnmarshalCode(
    assert(cont);
    if (useHelper)
    {
-        string helperName = getUnqualified(getNamespace(seq) + "." + seq->name() + "Helper", scope);
+        string helperName = getUnqualified(seq, scope, "", "Helper");


We already have an overload of getUnqualified which handles applying suffixes to types.

InsertCreativityHere · 2024-10-29T18:03:15Z

cpp/src/slice2swift/SwiftUtil.cpp

@@ -1099,7 +1094,6 @@ SwiftGenerator::typeToString(
    //
    string currentModule = getSwiftModule(getTopLevelModule(toplevel));
    BuiltinPtr builtin = dynamic_pointer_cast<Builtin>(type);
-    bool nonnull = find(metadata.begin(), metadata.end(), "swift:nonnull") != metadata.end();


According to the docs, nonnull was only for local operations:
https://doc.zeroc.com/ice/3.7/the-slice-language/slice-metadata-directives#id-.SliceMetadataDirectivesv3.7-swift:nonnull

InsertCreativityHere · 2024-10-29T18:03:35Z

cpp/src/slice2swift/SwiftUtil.cpp

-        if (s.find("swift:attribute:") == 0 || s.find("swift:type:") == 0 || s == "swift:noexcept" ||
-            s == "swift:nonnull")
-        {
-            dc->warning(InvalidMetadata, p->file(), p->line(), "ignoring metadata `" + s + "' for non local operation");


More local slice leftovers.

bernardnormier · 2024-10-29T18:19:26Z

cpp/src/Slice/Parser.h

+    // Metadata
+    // ----------------------------------------------------------------------
+
+    class Metadata final : public virtual GrammarBase


Can most likely remove virtual

Yep, leftover from a copy-paste.
Removed.

bernardnormier · 2024-10-29T18:20:12Z

cpp/src/Slice/Parser.h

+
+    private:
+        /// Parses a metadata string into a pair of (directive, arguments) strings.
+        static std::pair<std::string, std::string> parseRawMetadata(const std::string& rawMetadata);


string_view?

string.

Now the caller just forwards a string by moving it.
This lets us avoid a copy in the case where we don't want to split up the metadata at all,
and we can just return the string as-is, instead of needing to construct a new one from the string_view.

This lets us avoid a copy in the case where we don't want to split up the metadata at all,
and we can just return the string as-is, instead of needing to construct a new one from the string_view.

The API signature is about signaling intent to the caller, not about optimizing the implementation.

For example:

void addToMap(string, string); // implementation adopts args int parseInt(string_view); // implementation reads arg

For parsing, including this parseRawMetadata, the parameter should be a string_view. Implementation details like the implementation always making a working copy (as a string) or occasionally returning the arg unchanged (as a string) are just implementation details that should not change the API signature.

I went with a 3rd compromise option to save time debating here: I inlined the function.

I agree with you, but it didn't seem like we'd be able to agree on the intent of the parameter here.
I thought that by-value string (that it's free to modify however) was correct, but it's clear no one else did.

And when I switched it to string_view, it just hurt me too much to look at.
It was a private static helper function, called from only one place, that will be passed a string 100% of the time.
If switched to string_view, the first line copied it into a string anyways string x = string{my_string_view}.

bernardnormier · 2024-10-29T18:23:02Z

cpp/src/Slice/Parser.cpp

 }

 void
-Slice::DefinitionContext::setMetadata(const StringList& metadata)
+Slice::DefinitionContext::setMetadata(MetadataList metadata)


Missing std::move below. Check the other setMetadata too.

I checked through all the usage of setMetadata and added moves to the call-sites to avoid copies.

bernardnormier · 2024-10-29T18:25:44Z

cpp/src/slice2cs/Gen.cpp

        {
-            baseNames.push_back(q->substr(prefix.size()));
+            baseNames.push_back(string(metadata->arguments()));


Use {} for constructors

I fixed all the ones I introduced in this PR.
Ripgripping around shows that's still many other places this should be fixed of course.

externl · 2024-10-29T18:22:00Z

cpp/src/Slice/Parser.cpp

+    if (secondColonPos == string::npos)
+    {
+        // If the metadata contains only 1 colon, we need to check if it's for a language prefix, or for arguments.
+        // NOTE: It is important that this list is kept in alphabetical order!


And up to date! :)

When I get back I'll add a sticky note to update this list when we add any new languages to ice. : vP

externl · 2024-10-29T18:26:32Z

cpp/src/Slice/Parser.cpp

@@ -1458,9 +1525,9 @@ DictionaryPtr
 Slice::Container::createDictionary(
    const string& name,
    const TypePtr& keyType,
-    const StringList& keyMetadata,
+    MetadataList keyMetadata,


These are no longer by reference and should be std::moved into the dictionary constructor now.

externl · 2024-10-29T18:32:38Z

cpp/src/Slice/Parser.cpp

@@ -1542,7 +1609,7 @@ ConstPtr
 Slice::Container::createConst(
    const string name,
    const TypePtr& type,
-    const StringList& metadata,
+    MetadataList metadata,


Also fixed.

externl · 2024-10-29T18:33:48Z

cpp/src/Slice/Parser.cpp

@@ -4687,7 +4760,7 @@ Slice::Unit::currentIncludeLevel() const
 }

 void
-Slice::Unit::addFileMetadata(const StringList& metadata)
+Slice::Unit::addFileMetadata(const MetadataList& metadata)


Also fixed.

externl · 2024-10-29T18:33:58Z

cpp/src/Slice/Parser.cpp

@@ -4960,7 +5033,7 @@ Slice::Unit::getTopLevelModules(const string& file) const
    }
 }

-Slice::Unit::Unit(bool all, const StringList& defaultFileMetadata)
+Slice::Unit::Unit(bool all, MetadataList defaultFileMetadata)


Also fixed.

externl · 2024-10-29T18:47:52Z

cpp/src/Slice/Parser.h

+
+    private:
+        /// Parses a metadata string into a pair of (directive, arguments) strings.
+        static std::pair<std::string, std::string> parseRawMetadata(const std::string& rawMetadata);


should use string_view

Also ended up switching it to string by value.
That will save us a copy in the case where there is no ':', and we want to return the provided string as-is.

externl · 2024-10-29T18:48:27Z

cpp/src/Slice/Parser.h

+    class Metadata final : public virtual GrammarBase
+    {
+    public:
+        Metadata(const std::string& rawMetadata, const std::string& file, int line);


The stings should be non-const ref (or string_view) depending on what we do with them.

I switched them both to use string, since that lets us avoid making a copy from string_view -> string.

externl · 2024-10-29T18:48:53Z

cpp/src/Slice/Parser.h

+        std::string_view directive() const;
+        std::string_view arguments() const;
+
+        std::string file() const;


Why is this one not string_view?

Originally I did use a string_view, but everywhere where we called it was expecting a string.
Eventually wrapping it in a string constructor just got cluttered and annoying, so I switched it back.

Maybe some future refactoring of all the callers to this function can improve the situation and then it might be more ergonomic to switch it to string_view.

For accessors, returning a string (= copy) or a const string& (= const reference to private field) is fine, and can be preferable to a string_view. We prefer string_view over const string& for 'in' parameters.

pepone · 2024-10-30T10:07:30Z

cpp/src/Slice/Grammar.y

    auto contained = dynamic_pointer_cast<Contained>($3);
    if (contained && !metadata->v.empty())
    {
-        contained->setMetadata(metadata->v);
+        contained->setMetadata(std::move(metadata->v));


Is this std::move correct, moving a data-meber out of the object seems uncommon. Is the object not used anymore after this?

Yes, we never use the object again after this.

The only reason why we're using an object is because all the special $N types used by the parser must have a common base type (GrammarBase). So this object is just a type wrapper around the value that we're moving.

pepone · 2024-10-30T10:18:49Z

cpp/src/slice2cpp/Gen.cpp

            DefinitionContextPtr dc = unt->findDefinitionContext(file);
            assert(dc);
-            if (auto meta = dc->findMetadata(prefix))
+
+            // TODO: why do we ignore all instances of this metadata except the first?


I guess because this is to document the required include for the type, and it should never be more than one header

pepone · 2024-10-30T10:28:49Z

cpp/src/slice2cpp/Gen.cpp

 Slice::Gen::MetadataVisitor::validate(
    const SyntaxTreeBasePtr& cont,
-    const StringList& metadata,
+    const MetadataList& metadata,


pass by value and validate in place?

This would avoid a copy when we call this like:

p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));

I guess it would complicate the loop, as we have to use and iterator to remove items from the vector we are iterating... So maybe not worth the added complexity.

Implemented as suggested.
It doesn't make the loop much worse at all, except that we have to use iterators. Which is fine.
This is still C++ after all!

pepone · 2024-10-30T10:47:28Z

cpp/src/slice2swift/Gen.cpp

-    static const string prefix = "swift:inherits:";
-    for (StringList::const_iterator q = metadata.begin(); q != metadata.end(); ++q)
+    // Check for 'swift:inherits' metadata.
+    for (const auto& metadata : p->getMetadata())


Would be nice to be able to get a list of metadata for a directive, maybe we don't do this in many places?

We could definitely add it in the future, but indeed we don't need that functionality in many places, although there are a couple spots.

externl · 2024-10-30T13:58:51Z

cpp/src/Slice/Parser.cpp

@@ -126,7 +126,7 @@ Slice::Metadata::line() const
 }

 pair<string, string>
-Slice::Metadata::parseRawMetadata(const string& rawMetadata)
+Slice::Metadata::parseRawMetadata(string rawMetadata)


I think this one should be string_view :)

I went with a 3rd compromise option and just inlined the function in the one place it was used.
That way there's no signature type to argue about :)

InsertCreativityHere added 12 commits October 29, 2024 00:03

Checkpoint.

06bf69b

TEMP

b294813

hurricane checkpoint.

6f3b676

Regenerated grammar file.

9e31ef1

Bug fixes.

574f88a

Final checkpoint.

0ab0f91

bug fixes round 1.

542bf32

Bug fixes round 2.

4ef23b9

Finished.

86a9a51

On to the merge conflicts.

e4b77ed

Formatting...

f554b7c

Self review and added extra argument validation.

facd939

InsertCreativityHere requested review from pepone, externl and bernardnormier October 29, 2024 17:28

InsertCreativityHere commented Oct 29, 2024

View reviewed changes

Another formatting fix.

1a6895c

InsertCreativityHere force-pushed the make-metadata-great-again-2 branch from 6a518f0 to 1a6895c Compare October 29, 2024 18:05

bernardnormier approved these changes Oct 29, 2024

View reviewed changes

externl reviewed Oct 29, 2024

View reviewed changes

InsertCreativityHere added 2 commits October 29, 2024 18:15

Review fixes.

19debb6

Formatting.

de17b04

pepone approved these changes Oct 30, 2024

View reviewed changes

externl approved these changes Oct 30, 2024

View reviewed changes

More review fixes.

af95124

InsertCreativityHere force-pushed the make-metadata-great-again-2 branch from 61d2ae9 to af95124 Compare October 30, 2024 17:59

Typo.

7a8ba2f

InsertCreativityHere merged commit 465dc1b into zeroc-ice:main Oct 30, 2024
17 checks passed

Improve the Parser's API and Storage of Metadata #3003

Improve the Parser's API and Storage of Metadata #3003

Conversation

InsertCreativityHere commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InsertCreativityHere commented Oct 29, 2024 •

edited

Loading

bernardnormier Oct 30, 2024 •

edited

Loading

bernardnormier Oct 30, 2024 •

edited

Loading