Swap the Order that the Parser Creates 'Def' and 'Decl' Types #3359
Conversation
|
This no longer seems to trigger the Ice/exception Ruby bug anymore. |
| bool declared = dynamic_pointer_cast<InterfaceDecl>(matches.front()) != nullptr; | ||
| ostringstream os; | ||
| os << "class '" << name << "' was previously " << (declared ? "declared" : "defined") << " as " | ||
| << prependA(matches.front()->kindOf()); |
There was a problem hiding this comment.
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.
| ClassDeclPtr decl = createClassDecl(name); | ||
| def->_declaration = decl; | ||
| decl->_definition = def; |
There was a problem hiding this comment.
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.
| bool declared = dynamic_pointer_cast<InterfaceDecl>(matches.front()) != nullptr; | ||
| ostringstream os; | ||
| os << "class '" << name << "' was previously " << (declared ? "declared" : "defined") << " as " | ||
| << prependA(matches.front()->kindOf()); |
There was a problem hiding this comment.
Always say previously defined instead of previously declared
| def = dynamic_pointer_cast<ClassDef>(q); | ||
| assert(def); |
There was a problem hiding this comment.
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...
| if (def) | ||
| { | ||
| decl->_definition = def; | ||
| } |
There was a problem hiding this comment.
... And a result, def is always unset.
| 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 << "')"; | ||
| } |
There was a problem hiding this comment.
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.
| @@ -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"); | |||
There was a problem hiding this comment.
So many dead variables.
| @@ -1114,11 +1100,6 @@ Slice::Python::CodeVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p) | |||
| registerName(className); | |||
| _out.dec(); | |||
|
|
|||
| if (_classHistory.count(scoped) == 0) | |||
There was a problem hiding this comment.
Same but for InterfaceDecl.
| _out << nl << "if(!isset(" << type << "))"; | ||
| _out << sb; | ||
| _out << nl << type << " = IcePHP_declareClass('" << scoped << "');"; | ||
| _out << eb; |
There was a problem hiding this comment.
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.
| 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 << "');"; | ||
| } |
There was a problem hiding this comment.
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.
…s 'Def' and 'Decl' Types (zeroc-ice/ice#3359)
This PR is a long overdue follow up to my PRs which centralized the metadata validation.
There are two types which can be forward declared in Slice:
Interfaces andClasses.There are separate types for definitions (
InterfaceDefandClassDef) vs forward declarations (InterfaceDecl,ClassDecl).And, whenever the compiler creates a
Deftype, it also implicitly creates aDecltype to go with it.But, the fact that the compiler creates
Deffirst and thenDeclafter causes us to emit some errors/warnings in a weird order.This was only really noticed after implementing the metadata changes. I tried fixing it in that PR, but that caused an obscure ruby bug (#2442), so it was split off into a separate PR (this one).
Now, the compiler will create the
Decltypes before theDeftypes.This fixes the order of diagnostic messages, but it also lets us simplify some code in the compilers as well,
since whenever we look up a type by name, we are guaranteed to hit
Decls before we hitDefs.