Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Support for cpp:identifier Metadata, and the Groundwork for xxx:identifier Metadata in General #3292

Merged
merged 25 commits into from
Jan 15, 2025
Merged

Add Support for cpp:identifier Metadata, and the Groundwork for xxx:identifier Metadata in General #3292

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
6787ddb
The base structure is down.
InsertCreativityHere Dec 16, 2024
87e3dde
Implemented 'cpp:identifier'.
InsertCreativityHere Dec 17, 2024
11ac464
Turns out we actually needed this in our own code...
InsertCreativityHere Dec 17, 2024
78529c0
Fixing all the merge conflicts from clang-tidy...
InsertCreativityHere Dec 17, 2024
ce37faf
Enable the 'cpp:identifier' metadata in the validator.
InsertCreativityHere Dec 17, 2024
9cda559
Cleanup and formatting.
InsertCreativityHere Dec 17, 2024
b2e8390
Finish fixing the tests.
InsertCreativityHere Dec 18, 2024
5427090
Added a way to inject language prefix into the parser's core machinery.
InsertCreativityHere Jan 8, 2025
1cb67c5
Switch APIs to take bools instead of strings.
InsertCreativityHere Jan 8, 2025
329aeae
Expose the injected prefix with an accessor function.
InsertCreativityHere Jan 8, 2025
a3444f1
Need to change the dynamic compilation code in Ruby & Python.
InsertCreativityHere Jan 8, 2025
2c3b573
Remove support for putting this on modules.
InsertCreativityHere Jan 8, 2025
dca41be
Get the escape test working now that we can't fix module names anymore.
InsertCreativityHere Jan 8, 2025
3f54c9d
Self review stuff.
InsertCreativityHere Jan 8, 2025
382482e
Formatting...
InsertCreativityHere Jan 8, 2025
2e8b08d
Use separate functions instead of bools.
InsertCreativityHere Jan 13, 2025
3053c04
Missed a couple.
InsertCreativityHere Jan 13, 2025
c4b3647
Merge branch 'main' into xxx-identifier-metadata
InsertCreativityHere Jan 13, 2025
8a904fe
Lost one among the merge fixes.
InsertCreativityHere Jan 13, 2025
6eeb7ac
Review fixes and formatting.
InsertCreativityHere Jan 13, 2025
4084958
Overzealous.
InsertCreativityHere Jan 13, 2025
e013361
Formatting.
InsertCreativityHere Jan 14, 2025
3dc8134
Fixed review comments.
InsertCreativityHere Jan 14, 2025
951a19b
Merge branch 'main' into xxx-identifier-metadata
InsertCreativityHere Jan 15, 2025
3f92679
Merge conflict artifacts.
InsertCreativityHere Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cpp/src/Slice/MetadataValidation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,9 @@ Slice::validateMetadata(const UnitPtr& p, string_view prefix, map<string, Metada
MetadataInfo deprecatedInfo = {
.validOn =
{typeid(InterfaceDecl),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just swapped the order here to be consistent with other lists.

typeid(ClassDecl),
typeid(Operation),
typeid(Exception),
typeid(ClassDecl),
typeid(Slice::Exception),
typeid(Struct),
typeid(Sequence),
typeid(Dictionary),
Expand Down
62 changes: 33 additions & 29 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ compareTag(const T& lhs, const T& rhs)
return lhs->tag() < rhs->tag();
}

// NOTE: It is important that this list is kept in alphabetical order!
constexpr string_view languages[] = {"cpp", "cs", "java", "js", "matlab", "php", "python", "ruby", "swift"};

// Forward declare things from Bison and Flex the parser can use.
extern int slice_parse();
extern int slice_lineno;
Expand Down Expand Up @@ -104,8 +107,6 @@ Slice::Metadata::Metadata(string rawMetadata, string file, int line) : GrammarBa
if (firstColonPos != string::npos)
{
// Check if the metadata starts with a language prefix.
// NOTE: It is important that this list is kept in alphabetical order!
constexpr string_view languages[] = {"cpp", "cs", "java", "js", "matlab", "php", "python", "rb", "swift"};
string prefix = rawMetadata.substr(0, firstColonPos);
bool hasLangPrefix = binary_search(&languages[0], &languages[sizeof(languages) / sizeof(*languages)], prefix);
if (hasLangPrefix)
Expand Down Expand Up @@ -554,35 +555,37 @@ Slice::Contained::container() const
}

string
Slice::Contained::name() const
Slice::Contained::name(bool mappedName) const
{
return _name;
}
// If the mapped name was requested, we first check if any 'xxx:identifier' has been applied to this element.
// If so, we return that instead of the element's Slice identifier.
if (mappedName)
{
static const string metadata = string(_unit->languagePrefix()) + ":identifier";
if (auto customName = getMetadataArgs(metadata))
{
return *customName;
}
}

string
Slice::Contained::scoped() const
{
return _scoped;
return _name;
}

string
Slice::Contained::scope() const
Slice::Contained::scoped(bool mappedName) const
{
string::size_type idx = _scoped.rfind("::");
assert(idx != string::npos);
return string(_scoped, 0, idx + 2);
return scope(mappedName) + name(mappedName);
}

string
Slice::Contained::flattenedScope() const
Slice::Contained::scope(bool mappedName) const
{
string s = scope();
string::size_type pos = 0;
while ((pos = s.find("::", pos)) != string::npos)
string scoped;
if (auto container = dynamic_pointer_cast<Contained>(_container))
{
s.replace(pos, 2, "_");
scoped = container->scoped(mappedName);
}
return s;
return scoped + "::";
}

string
Expand Down Expand Up @@ -1050,12 +1053,6 @@ Slice::Contained::Contained(const ContainerPtr& container, const string& name)
_container(container),
_name(name)
{
ContainedPtr cont = dynamic_pointer_cast<Contained>(_container);
if (cont)
{
_scoped = cont->scoped();
}
_scoped += "::" + _name;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't much point to _scoped now.

Most of the calls to scoped() and scope() are for mapped identifiers, and now we have to compute their values on the fly. So, there's even less point to caching the _scoped name up-front. So, I removed it.

assert(_unit);
_file = _unit->currentFile();
_line = _unit->currentLine();
Expand Down Expand Up @@ -4555,19 +4552,25 @@ Slice::DataMember::DataMember(
// ----------------------------------------------------------------------
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the code below this is to allow the compilers to inject their languagePrefix into libSlice.


UnitPtr
Slice::Unit::createUnit(bool all, const StringList& defaultFileMetadata)
Slice::Unit::createUnit(string_view languagePrefix, bool all, const StringList& defaultFileMetadata)
{
MetadataList defaultMetadata;
for (const auto& metadataString : defaultFileMetadata)
{
defaultMetadata.push_back(make_shared<Metadata>(metadataString, "<command-line>", 0));
}

UnitPtr unit{new Unit{all, std::move(defaultMetadata)}};
UnitPtr unit{new Unit{languagePrefix, all, std::move(defaultMetadata)}};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code above is a mystery to me. Would be nice to add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not my code, but as I read it, we're creating a cyclic shared_ptr here.
Which is just leaking memory, right?

Copy link
Member

@pepone pepone Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SyntaxTreeBase has a _unit, and Unit is a SyntaxTreeBase so it needs this _unit.

We can consider replacing this by a virtual unit() method, which for Unit can return shared_from_this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think another option is just not having Unit inherit from SyntaxTreeBase.
It's a pretty small API surface, most of which isn't useful for Unit:

virtual void destroy();                   // Only sets `_unit = nullptr;`
UnitPtr unit() const;                     // Useless on `Unit`, it already has access to itself.
DefinitionContextPtr definitionContext() const;     // Always `nullptr` for `Unit`
virtual void visit(ParserVisitor* visitor);         // Only useful function

So, it'd actually be nice to remove this useless inheritance, or simplify it somehow.

Unfortunately, we use SyntaxTreeBasePtr as a base type for many places, so this would require a good deal of reshuffling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking about this code:

    MetadataList defaultMetadata;
    for (const auto& metadataString : defaultFileMetadata)
    {
        defaultMetadata.push_back(make_shared<Metadata>(metadataString, "<command-line>", 0));
    }

unit->_unit = unit;
return unit;
}

string_view
Slice::Unit::languagePrefix() const
{
return _languagePrefix;
}

void
Slice::Unit::setDocComment(const string& comment)
{
Expand Down Expand Up @@ -5017,15 +5020,16 @@ Slice::Unit::getTopLevelModules(const string& file) const
}
}

Slice::Unit::Unit(bool all, MetadataList defaultFileMetadata)
Slice::Unit::Unit(std::string_view languagePrefix, bool all, MetadataList defaultFileMetadata)
: SyntaxTreeBase(nullptr),
Container(nullptr),
_languagePrefix(languagePrefix),
_all(all),
_defaultFileMetadata(std::move(defaultFileMetadata)),
_errors(0),
_currentIncludeLevel(0)

{
assert(binary_search(&languages[0], &languages[sizeof(languages) / sizeof(*languages)], _languagePrefix));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work for 'icerpc'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't, but I fixed it. And opened an issue to run ice2slice in CI.
Right now, we have 0 testing of this tool, and it's way too easy to break it without realizing...

}

// ----------------------------------------------------------------------
Expand Down
20 changes: 12 additions & 8 deletions cpp/src/Slice/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,10 +363,11 @@ namespace Slice
{
public:
[[nodiscard]] ContainerPtr container() const;
[[nodiscard]] std::string name() const;
[[nodiscard]] std::string scoped() const;
[[nodiscard]] std::string scope() const;
[[nodiscard]] std::string flattenedScope() const;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flattenedScope was only used by slice2cpp, so I moved the logic into that compiler.


[[nodiscard]] std::string name(bool mappedName = false) const;
[[nodiscard]] std::string scoped(bool mappedName = false) const;
[[nodiscard]] std::string scope(bool mappedName = false) const;

[[nodiscard]] std::string file() const;
[[nodiscard]] int line() const;

Expand Down Expand Up @@ -401,7 +402,6 @@ namespace Slice

ContainerPtr _container;
std::string _name;
std::string _scoped;
std::string _file;
int _line;
std::string _docComment;
Expand Down Expand Up @@ -453,7 +453,6 @@ namespace Slice
EnumeratorList enumerators() const;
EnumeratorList enumerators(const std::string& identifier) const;
ContainedList contents() const;
std::string thisScope() const;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is only used internally.
Instead of keeping it public and adding the mappedName flag, I just made it protected, so only things in libSlice (which don't care about mapped names) can access it.

void visit(ParserVisitor* visitor) override;

bool checkIntroduced(const std::string& scopedName, ContainedPtr namedThing = nullptr);
Expand Down Expand Up @@ -483,6 +482,8 @@ namespace Slice
}

protected:
std::string thisScope() const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing [nodiscard]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.


bool validateConstant(
const std::string& name,
const TypePtr& type,
Expand Down Expand Up @@ -988,7 +989,9 @@ namespace Slice
class Unit final : public virtual Container
{
public:
static UnitPtr createUnit(bool all, const StringList& defaultFileMetadata = StringList());
static UnitPtr createUnit(std::string_view languagePrefix, bool all, const StringList& defaultFileMetadata = StringList());

[[nodiscard]] std::string_view languagePrefix() const;

void setDocComment(const std::string& comment);
void addToDocComment(const std::string& comment);
Expand Down Expand Up @@ -1040,11 +1043,12 @@ namespace Slice
std::set<std::string> getTopLevelModules(const std::string& file) const;

private:
Unit(bool all, MetadataList defaultFileMetadata);
Unit(std::string_view languagePrefix, bool all, MetadataList defaultFileMetadata);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this use of string_view over the top. Using strings is preferable, especially for something you adopt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, I switched it to use string instead.


void pushDefinitionContext();
void popDefinitionContext();

const std::string_view _languagePrefix;
bool _all;
MetadataList _defaultFileMetadata;
int _errors;
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/ice2slice/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ compile(const vector<string>& argv)
}
else
{
UnitPtr p = Unit::createUnit(false);
UnitPtr p = Unit::createUnit("icerpc", false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work since icerpc is not a mapped language in Parser.cpp?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't. Good catch that I forgot to add it to the list of known languages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is icerpc meaningful here? What does this "prefix" mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think adding 'icerpc' to the list of known language mappings is a good fix. You could make this first parameter std::optional or treat "" as a special value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed icerpc. Now ice2slice just passes the empty string.
And I added an assert to mappedName which enforces that the language string is non-empty.
Since ice2slice should never be using mapped names anyways.

int parseStatus = p->parse(*i, cppHandle, debug);

if (!icecpp->close())
Expand Down
Loading
Loading