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

Swap the Order that the Parser Creates 'Def' and 'Decl' Types #3359

Merged
merged 7 commits into from
Jan 15, 2025
Merged

Swap the Order that the Parser Creates 'Def' and 'Decl' Types #3359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6e41c52
Swap the order that the parser creates 'Def' and 'Decl' types.
InsertCreativityHere Jan 14, 2025
877cb15
Cross link the definitions and declarations.
InsertCreativityHere Jan 14, 2025
64da316
Simplify error message.
InsertCreativityHere Jan 14, 2025
3b18c5d
Remove some unused variables.
InsertCreativityHere Jan 14, 2025
f231569
Fixed placement of warning message.
InsertCreativityHere Jan 14, 2025
ef3f744
Merge branch 'main' into swap-def-decl-creation-order
InsertCreativityHere Jan 14, 2025
f6fac9c
Merge branch 'main' into swap-def-decl-creation-order
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
74 changes: 22 additions & 52 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,10 +1173,8 @@ Slice::Container::createClassDef(const string& name, int id, const ClassDefPtr&
}
else
{
bool declared = dynamic_pointer_cast<InterfaceDecl>(matches.front()) != nullptr;
ostringstream os;
os << "class '" << name << "' was previously " << (declared ? "declared" : "defined") << " as "
<< prependA(matches.front()->kindOf());
Comment on lines -1176 to -1179
Copy link
Member Author

Choose a reason for hiding this comment

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

When discussing this before we decided to no longer emit
was previously declared vs was previously defined based on whether we hit a forward declaration. Now we always say was previously defined.

os << "class '" << name << "' was previously defined as " << prependA(matches.front()->kindOf());
_unit->error(os.str());
}
return nullptr;
Expand All @@ -1187,29 +1185,27 @@ Slice::Container::createClassDef(const string& name, int id, const ClassDefPtr&
return nullptr;
}

// Implicitly create a class declaration for each class definition.
// This way the code generator can rely on always having a class declaration available for lookup.
ClassDefPtr def = make_shared<ClassDef>(shared_from_this(), name, id, base);
_unit->addContent(def);
_contents.push_back(def);
ClassDeclPtr decl = createClassDecl(name);
def->_declaration = decl;
decl->_definition = def;
Comment on lines +1191 to +1193
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 the main part of this PR. We want to call createXXXDecl before we do:

    _unit->addContent(def);
    _contents.push_back(def);

So that Decls come before Defs in the _contents list.


// Patch forward declarations which may of been created in other openings of this class' module.
for (const auto& q : matches)
{
ClassDeclPtr decl = dynamic_pointer_cast<ClassDecl>(q);
decl->_definition = def;
dynamic_pointer_cast<ClassDecl>(q)->_definition = def;
}

// Implicitly create a class declaration for each class definition.
// This way the code generator can rely on always having a class declaration available for lookup.
ClassDeclPtr decl = createClassDecl(name);
def->_declaration = decl;

_unit->addContent(def);
_contents.push_back(def);
return def;
}

ClassDeclPtr
Slice::Container::createClassDecl(const string& name)
{
ClassDefPtr def;

ContainedList matches = _unit->findContents(thisScope() + name);
for (const auto& p : matches)
{
Expand All @@ -1235,10 +1231,8 @@ Slice::Container::createClassDecl(const string& name)
}
else
{
bool declared = dynamic_pointer_cast<InterfaceDecl>(matches.front()) != nullptr;
ostringstream os;
os << "class '" << name << "' was previously " << (declared ? "declared" : "defined") << " as "
<< prependA(matches.front()->kindOf());
Comment on lines -1238 to -1241
Copy link
Member Author

Choose a reason for hiding this comment

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

Always say previously defined instead of previously declared

os << "class '" << name << "' was previously defined as " << prependA(matches.front()->kindOf());
_unit->error(os.str());
}
return nullptr;
Expand All @@ -1263,21 +1257,12 @@ Slice::Container::createClassDecl(const string& name)
{
return decl;
}

def = dynamic_pointer_cast<ClassDef>(q);
assert(def);
Comment on lines -1267 to -1268
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 impossible to hit.
Whenever we see ClassDef, we know there's a ClassDecl right before it in the 'ast'.

And the line above this is basically:
if we see a ClassDecl, return it to avoid creating a new ClassDecl.

So, even if a ClassDef exists, we will always hit the ClassDecl branch first...

}
}

ClassDeclPtr decl = make_shared<ClassDecl>(shared_from_this(), name);
_unit->addContent(decl);
_contents.push_back(decl);

if (def)
{
decl->_definition = def;
}
Comment on lines -1276 to -1279
Copy link
Member Author

Choose a reason for hiding this comment

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

... And a result, def is always unset.


return decl;
}

Expand Down Expand Up @@ -1320,10 +1305,8 @@ Slice::Container::createInterfaceDef(const string& name, const InterfaceList& ba
}
else
{
Copy link
Member Author

Choose a reason for hiding this comment

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

Same idea as the code above, except for InterfaceDecl now.

bool declared = dynamic_pointer_cast<ClassDecl>(matches.front()) != nullptr;
ostringstream os;
os << "interface '" << name << "' was previously " << (declared ? "declared" : "defined") << " as "
<< prependA(matches.front()->kindOf());
os << "interface '" << name << "' was previously defined as " << prependA(matches.front()->kindOf());
_unit->error(os.str());
}
return nullptr;
Expand All @@ -1336,29 +1319,27 @@ Slice::Container::createInterfaceDef(const string& name, const InterfaceList& ba

InterfaceDecl::checkBasesAreLegal(name, bases, _unit);

// Implicitly create an interface declaration for each interface definition.
// This way the code generator can rely on always having an interface declaration available for lookup.
InterfaceDefPtr def = make_shared<InterfaceDef>(shared_from_this(), name, bases);
_unit->addContent(def);
_contents.push_back(def);
InterfaceDeclPtr decl = createInterfaceDecl(name);
def->_declaration = decl;
decl->_definition = def;

// Patch forward declarations which may of been created in other openings of this interface's module.
for (const auto& q : matches)
{
InterfaceDeclPtr decl = dynamic_pointer_cast<InterfaceDecl>(q);
decl->_definition = def;
dynamic_pointer_cast<InterfaceDecl>(q)->_definition = def;
}

// Implicitly create an interface declaration for each interface definition.
// This way the code generator can rely on always having an interface declaration available for lookup.
InterfaceDeclPtr decl = createInterfaceDecl(name);
def->_declaration = decl;

_unit->addContent(def);
_contents.push_back(def);
return def;
}

InterfaceDeclPtr
Slice::Container::createInterfaceDecl(const string& name)
{
InterfaceDefPtr def;

ContainedList matches = _unit->findContents(thisScope() + name);
for (const auto& p : matches)
{
Expand All @@ -1384,10 +1365,8 @@ Slice::Container::createInterfaceDecl(const string& name)
}
else
{
bool declared = dynamic_pointer_cast<ClassDecl>(matches.front()) != nullptr;
ostringstream os;
os << "interface '" << name << "' was previously " << (declared ? "declared" : "defined") << " as "
<< prependA(matches.front()->kindOf());
os << "interface '" << name << "' was previously defined as " << prependA(matches.front()->kindOf());
_unit->error(os.str());
}
return nullptr;
Expand All @@ -1409,21 +1388,12 @@ Slice::Container::createInterfaceDecl(const string& name)
{
return decl;
}

def = dynamic_pointer_cast<InterfaceDef>(q);
assert(def);
}
}

InterfaceDeclPtr decl = make_shared<InterfaceDecl>(shared_from_this(), name);
_unit->addContent(decl);
_contents.push_back(decl);

if (def)
{
decl->_definition = def;
}

return decl;
}

Expand Down
2 changes: 0 additions & 2 deletions cpp/src/slice2cpp/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1057,9 +1057,7 @@ Slice::Gen::ForwardDeclVisitor::visitModuleEnd(const ModulePtr&)
void
Slice::Gen::ForwardDeclVisitor::visitClassDecl(const ClassDeclPtr& p)
{
ClassDefPtr def = p->definition();
Copy link
Member Author

Choose a reason for hiding this comment

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

More dead variables.
There's got to be some linter/tool for removing these things right?

string name = fixKwd(p->name());

H << nl << "class " << name << ';';
H << nl << "using " << p->name() << "Ptr " << getDeprecatedAttribute(p) << "= ::std::shared_ptr<" << name << ">;"
<< sp;
Expand Down
23 changes: 0 additions & 23 deletions cpp/src/slice2php/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,7 @@ CodeVisitor::visitClassDecl(const ClassDeclPtr& p)

string type = getTypeVar(p);
_out << sp << nl << "global " << type << ';';

_out << nl << "if(!isset(" << type << "))";
_out << sb;
_out << nl << type << " = IcePHP_declareClass('" << scoped << "');";
_out << eb;
Comment on lines -135 to -138
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 believe that this check for isset is unnecessary.
This block is wrapped in an if statement:

if (_classHistory.count(scoped) == 0)
{...
_classHistory.insert(scoped); // Avoid redundant declarations.
}

which should ensure that this code is only generated once.

Even if I'm wrong (which I don't think I am), there's nothing wrong with setting type = IcePHP_declareClass multiple times, it's just redundant is all.


endNamespace();

Expand All @@ -156,10 +152,7 @@ CodeVisitor::visitInterfaceDecl(const InterfaceDeclPtr& p)
_out << sp << nl << "global " << type << ';';

_out << nl << "global " << type << "Prx;";
_out << nl << "if(!isset(" << type << "))";
_out << sb;
_out << nl << type << "Prx = IcePHP_declareProxy('" << scoped << "');";
_out << eb;

endNamespace();

Expand Down Expand Up @@ -271,12 +264,6 @@ CodeVisitor::visitClassDefStart(const ClassDefPtr& p)

_out << eb; // End of class.

if (_classHistory.count(scoped) == 0 && p->canBeCyclic())
{
// Emit a forward declaration for the class in case a data member refers to this type.
_out << sp << nl << type << " = IcePHP_declareClass('" << scoped << "');";
}
Comment on lines -274 to -278
Copy link
Member Author

Choose a reason for hiding this comment

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

Impossible to hit.
visitClassDecl also checks for _classHistory.count(scoped) == 0 and inserts scoped if so. Since we always call visitClassDecl before visitClassDef, by the time we get here, the count will always be non-zero.


{
string type;
vector<string> seenType;
Expand Down Expand Up @@ -348,11 +335,6 @@ CodeVisitor::visitClassDefStart(const ClassDefPtr& p)

endNamespace();

if (_classHistory.count(scoped) == 0)
{
_classHistory.insert(scoped); // Avoid redundant declarations.
}

return false;
}

Expand Down Expand Up @@ -583,11 +565,6 @@ CodeVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)

endNamespace();

if (_classHistory.count(scoped) == 0)
{
_classHistory.insert(scoped); // Avoid redundant declarations.
}

return false;
}

Expand Down
21 changes: 1 addition & 20 deletions cpp/src/slice2py/PythonUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,6 @@ Slice::Python::CodeVisitor::visitClassDefStart(const ClassDefPtr& p)
{
string scoped = p->scoped();
string type = getAbsolute(p, "_t_");
string classType = getAbsolute(p, "_t_", "Disp");
Copy link
Member Author

Choose a reason for hiding this comment

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

So many dead variables.

string abs = getAbsolute(p);
string valueName = fixIdent(p->name());
ClassDefPtr base = p->base();
Expand Down Expand Up @@ -639,13 +638,6 @@ Slice::Python::CodeVisitor::visitClassDefStart(const ClassDefPtr& p)

_out.dec();

if (_classHistory.count(scoped) == 0 && p->canBeCyclic())
{
//
// Emit a forward declaration for the class in case a data member refers to this type.
//
_out << sp << nl << "_M_" << type << " = IcePy.declareValue('" << scoped << "')";
}
Comment on lines -642 to -648
Copy link
Member Author

Choose a reason for hiding this comment

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

Impossible to hit.

visitClassDecl also has this logic, and at the end, adds scoped to _classHistory.
Now that we're guaranteed to call visitClassDecl before visitClassDef,
_classHistory.count(scoped) == 0 will always be false.

visitInterfaceDecl also has this logic. And since we're guaranteed to call visitInterfaceDecl before a call to visitInterfaceDef, _classHistory will always contain this element by this point.

DataMemberList members = p->dataMembers();
_out << sp << nl << "_M_" << type << " = IcePy.defineValue('" << scoped << "', " << valueName << ", "
<< p->compactId() << ", ";
Expand Down Expand Up @@ -708,19 +700,13 @@ Slice::Python::CodeVisitor::visitClassDefStart(const ClassDefPtr& p)

_out.dec();

if (_classHistory.count(scoped) == 0)
{
_classHistory.insert(scoped); // Avoid redundant declarations.
}

return false;
}

bool
Slice::Python::CodeVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
{
string scoped = p->scoped();
string type = getAbsolute(p, "_t_");
string classType = getAbsolute(p, "_t_", "Disp");
string abs = getAbsolute(p);
string className = fixIdent(p->name());
Expand Down Expand Up @@ -977,7 +963,7 @@ Slice::Python::CodeVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
//
_out << sp << nl << "def __str__(self):";
_out.inc();
_out << nl << "return IcePy.stringify(self, _M_" << getAbsolute(p, "_t_", "Disp") << ")";
_out << nl << "return IcePy.stringify(self, _M_" << classType << ")";
_out.dec();
_out << sp << nl << "__repr__ = __str__";

Expand Down Expand Up @@ -1114,11 +1100,6 @@ Slice::Python::CodeVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
registerName(className);
_out.dec();

if (_classHistory.count(scoped) == 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

Same but for InterfaceDecl.

{
_classHistory.insert(scoped); // Avoid redundant declarations.
}

return false;
}

Expand Down
23 changes: 0 additions & 23 deletions cpp/src/slice2rb/RubyUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,17 +370,6 @@ Slice::Ruby::CodeVisitor::visitClassDefStart(const ClassDefPtr& p)
_out.dec();
_out << nl << "end"; // End of class.

//
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 exact same code is executed by visitClassDecl right above this.

And now that we're guaranteed to run visitClassDecl before visitClassDef (the function you're looking at right now) this code is redundant.

// Emit type descriptions.
//
_out << sp << nl << "if not defined?(" << getAbsolute(p, IdentToUpper, "T_");
_out << ')';
_out.inc();
_out << nl << "T_" << name << " = ::Ice::__declareClass('" << scoped << "')";
_out.dec();
_out << nl << "end";
_classHistory.insert(scoped); // Avoid redundant declarations.

_out << sp << nl << "T_" << name << ".defineClass(" << name << ", " << p->compactId() << ", "
<< "false, ";
if (!base)
Expand Down Expand Up @@ -502,18 +491,6 @@ Slice::Ruby::CodeVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
_out.dec();
_out << nl << "end"; // End of proxy class.

//
Copy link
Member Author

Choose a reason for hiding this comment

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

Same but for InterfaceDecl.

// Emit type descriptions.
//
_out << sp << nl << "if not defined?(" << getAbsolute(p, IdentToUpper, "T_");
_out << "Prx";
_out << ')';
_out.inc();
_out << nl << "T_" << name << "Prx = ::Ice::__declareProxy('" << scoped << "')";
_out.dec();
_out << nl << "end";
_classHistory.insert(scoped); // Avoid redundant declarations.

//
// Define each operation. The arguments to __defineOperation are:
//
Expand Down
1 change: 1 addition & 0 deletions cpp/src/slice2swift/SwiftUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ Slice::getTopLevelModule(const ContainedPtr& cont)
ModulePtr
Slice::getTopLevelModule(const TypePtr& type)
{
// TODO this check is redundant. `InterfaceDecl` _is_ a `Contained`. This whole function is weird though.
assert(dynamic_pointer_cast<InterfaceDecl>(type) || dynamic_pointer_cast<Contained>(type));

InterfaceDeclPtr proxy = dynamic_pointer_cast<InterfaceDecl>(type);
Expand Down
8 changes: 4 additions & 4 deletions cpp/test/Slice/errorDetection/InterfaceMismatch.err
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
InterfaceMismatch.ice:9: class 'Foo1' was previously declared as an interface
InterfaceMismatch.ice:10: class 'Foo1' was previously declared as an interface
InterfaceMismatch.ice:9: class 'Foo1' was previously defined as an interface
InterfaceMismatch.ice:10: class 'Foo1' was previously defined as an interface
InterfaceMismatch.ice:13: class 'Foo2' was previously defined as an interface
InterfaceMismatch.ice:16: interface 'Foo3' was previously declared as a class
InterfaceMismatch.ice:17: interface 'Foo3' was previously declared as a class
InterfaceMismatch.ice:16: interface 'Foo3' was previously defined as a class
InterfaceMismatch.ice:17: interface 'Foo3' was previously defined as a class
InterfaceMismatch.ice:20: interface 'Foo4' was previously defined as a class
2 changes: 1 addition & 1 deletion cpp/test/Slice/errorDetection/WarningInvalidMetadata.err
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ WarningInvalidMetadata.ice:31: warning: the 'cpp:no-default-include' metadata do
WarningInvalidMetadata.ice:34: warning: 'amd' metadata cannot be specified as file metadata
WarningInvalidMetadata.ice:40: warning: the 'cpp:header-ext' metadata only accepts one argument but a list was provided
WarningInvalidMetadata.ice:40: warning: 'cpp:header-ext' metadata cannot be applied to sequences
WarningInvalidMetadata.ice:44: warning: ignoring duplicate metadata: 'deprecated:do not use this' does not match previously applied metadata of 'deprecated'
Copy link
Member Author

Choose a reason for hiding this comment

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

And finally, the original reason for doing this:
now all diagnostics are in line-number ordering.

WarningInvalidMetadata.ice:48: warning: ignoring unknown metadata: 'unknown'
WarningInvalidMetadata.ice:48: warning: ignoring unknown metadata: 'cpp:unknown'
WarningInvalidMetadata.ice:48: warning: ignoring unknown metadata: 'bad:unknown'
WarningInvalidMetadata.ice:56: warning: the 'protected' metadata does not take any arguments
WarningInvalidMetadata.ice:56: warning: the 'cpp:array' metadata does not take any arguments
WarningInvalidMetadata.ice:56: warning: 'cpp:array' metadata can only be applied to operation parameters and return types
WarningInvalidMetadata.ice:44: warning: ignoring duplicate metadata: 'deprecated:do not use this' does not match previously applied metadata of 'deprecated'
WarningInvalidMetadata.ice:62: warning: 'cpp:type' metadata cannot be applied to operations with void return type
WarningInvalidMetadata.ice:65: warning: 'cpp:array' metadata cannot be applied to operations with void return type
WarningInvalidMetadata.ice:68: warning: invalid argument 'my_string' supplied to 'cpp:type' metadata in this context
Expand Down
Loading