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

Unit Cycle Fixes & Cleanup SyntaxTreeBase #3496

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Unit Cycle Fixes & Cleanup SyntaxTreeBase #3496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e473575
Removed redundant 'unit' from 'Contained' and made 'unit()' non-const.
InsertCreativityHere Feb 4, 2025
9f46940
Use 'unit()' function instead of '_unit' field.
InsertCreativityHere Feb 4, 2025
31ff3ee
The meat of the PR, switching the unit functions.
InsertCreativityHere Feb 4, 2025
9d137ab
Cleaned up the 'destroy' functions too.
InsertCreativityHere Feb 4, 2025
16de91e
Buf fix.
InsertCreativityHere Feb 4, 2025
b2c4711
Added the necessary 'destroy' calls.
InsertCreativityHere Feb 4, 2025
cca131d
Make 'visit' a pure virtual function.
InsertCreativityHere Feb 4, 2025
7abb057
Move 'visit' up from 'SyntaxTreeBase' to 'Contained'.
InsertCreativityHere Feb 4, 2025
464408b
Formatting.
InsertCreativityHere Feb 4, 2025
51f26cc
Initialization order.
InsertCreativityHere Feb 4, 2025
d0217b0
We need to call 'destroy' on the 'Builtin' types now.
InsertCreativityHere Feb 4, 2025
452bffd
Remove 'destroy' from 'SyntaxTreeBase'. Each type implements it on it…
InsertCreativityHere Feb 4, 2025
cf67620
Remove intermediate 'Constructed' type.
InsertCreativityHere Feb 4, 2025
4cc4f1d
Remove a little virtual inheritance at least.
InsertCreativityHere Feb 4, 2025
652106f
Resolved the warning we were ignoring.
InsertCreativityHere Feb 4, 2025
6312163
Lint fixes: we can take 'UnitPtr' by value now.
InsertCreativityHere Feb 4, 2025
d3a4ad4
No longer need 'virtual' for the only-contained types.
InsertCreativityHere Feb 4, 2025
6410774
Removed all the remaining virtual inheritance except for 'Container',…
InsertCreativityHere Feb 4, 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
482 changes: 224 additions & 258 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop

Large diffs are not rendered by default.

93 changes: 42 additions & 51 deletions cpp/src/Slice/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ namespace Slice
class Contained;
class Container;
class Module;
class Constructed;
class ClassDecl;
class ClassDef;
class InterfaceDecl;
Expand All @@ -75,7 +74,6 @@ namespace Slice
using ContainedPtr = std::shared_ptr<Contained>;
using ContainerPtr = std::shared_ptr<Container>;
using ModulePtr = std::shared_ptr<Module>;
using ConstructedPtr = std::shared_ptr<Constructed>;
using ClassDeclPtr = std::shared_ptr<ClassDecl>;
using ClassDefPtr = std::shared_ptr<ClassDef>;
using InterfaceDeclPtr = std::shared_ptr<InterfaceDecl>;
Expand Down Expand Up @@ -310,13 +308,7 @@ namespace Slice
class SyntaxTreeBase : public GrammarBase
{
public:
SyntaxTreeBase(UnitPtr unit);
virtual void destroy();
[[nodiscard]] UnitPtr unit() const;
virtual void visit(ParserVisitor* visitor);

protected:
UnitPtr _unit;
[[nodiscard]] virtual UnitPtr unit() = 0;
};

// ----------------------------------------------------------------------
Expand All @@ -326,7 +318,6 @@ namespace Slice
class Type : public virtual SyntaxTreeBase
{
public:
Type(const UnitPtr& unit);
[[nodiscard]] virtual bool isClassType() const;
[[nodiscard]] virtual bool usesClasses() const;
[[nodiscard]] virtual size_t minWireSize() const = 0;
Expand All @@ -338,7 +329,7 @@ namespace Slice
// Builtin
// ----------------------------------------------------------------------

class Builtin final : public virtual Type
class Builtin final : public Type
{
public:
enum Kind
Expand All @@ -356,7 +347,9 @@ namespace Slice
KindValue
};

Builtin(const UnitPtr& unit, Kind kind);
Builtin(UnitPtr unit, Kind kind);
void destroy();
Copy link
Member

@bernardnormier bernardnormier Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand the logic of destroy(). Which cycle are you breaking?

Update: ok, it's because Unit holds onto a list of Builtins. The modern approach is for the Builtin to hold a weak_ptr<Unit>. This way, no cycle and no need for Builtin::destroy.

[[nodiscard]] UnitPtr unit() final;

[[nodiscard]] bool isClassType() const final;
[[nodiscard]] size_t minWireSize() const final;
Expand All @@ -376,6 +369,7 @@ namespace Slice

private:
const Kind _kind;
UnitPtr _unit;
};

// ----------------------------------------------------------------------
Expand All @@ -385,6 +379,7 @@ namespace Slice
class Contained : public virtual SyntaxTreeBase
{
public:
virtual void destroy();
[[nodiscard]] ContainerPtr container() const;

/// Returns the Slice identifier of this element.
Expand All @@ -409,6 +404,7 @@ namespace Slice

[[nodiscard]] int includeLevel() const;
[[nodiscard]] DefinitionContextPtr definitionContext() const;
[[nodiscard]] UnitPtr unit() final;

[[nodiscard]] virtual MetadataList getMetadata() const;
virtual void setMetadata(MetadataList metadata);
Expand All @@ -427,6 +423,7 @@ namespace Slice
/// @return The message provided to the 'deprecated' metadata, if present.
[[nodiscard]] std::optional<std::string> getDeprecationReason() const;

virtual void visit(ParserVisitor* visitor) = 0;
[[nodiscard]] virtual std::string kindOf() const = 0;

protected:
Expand All @@ -440,6 +437,9 @@ namespace Slice
int _includeLevel;
DefinitionContextPtr _definitionContext;
MetadataList _metadata;

private:
UnitPtr _unit;
};

// ----------------------------------------------------------------------
Expand All @@ -449,8 +449,7 @@ namespace Slice
class Container : public virtual SyntaxTreeBase, public std::enable_shared_from_this<Container>
{
public:
Container(const UnitPtr& unit);
void destroy() override;
void destroyContents();
ModulePtr createModule(const std::string& name);
[[nodiscard]] ClassDefPtr createClassDef(const std::string& name, int id, const ClassDefPtr& base);
[[nodiscard]] ClassDeclPtr createClassDecl(const std::string& name);
Expand Down Expand Up @@ -481,14 +480,13 @@ namespace Slice
lookupTypeNoBuiltin(const std::string& identifier, bool emitErrors, bool ignoreUndefined = false);
[[nodiscard]] ContainedList lookupContained(const std::string& identifier, bool emitErrors);
[[nodiscard]] ExceptionPtr lookupException(const std::string& identifier, bool emitErrors);
[[nodiscard]] UnitPtr unit() const;
[[nodiscard]] ModuleList modules() const;
[[nodiscard]] InterfaceList interfaces() const;
[[nodiscard]] EnumList enums() const;
[[nodiscard]] EnumeratorList enumerators() const;
[[nodiscard]] EnumeratorList enumerators(const std::string& identifier) const;
[[nodiscard]] ContainedList contents() const;
void visit(ParserVisitor* visitor) override;
void visitContents(ParserVisitor* visitor);

bool checkIntroduced(const std::string& scopedName, ContainedPtr namedThing = nullptr);

Expand Down Expand Up @@ -534,29 +532,20 @@ namespace Slice
// Module
// ----------------------------------------------------------------------

class Module final : public virtual Container, public virtual Contained
class Module final : public Container, public Contained
{
public:
Module(const ContainerPtr& container, const std::string& name);
[[nodiscard]] std::string kindOf() const final;
void visit(ParserVisitor* visitor) final;
};

// ----------------------------------------------------------------------
// Constructed
// ----------------------------------------------------------------------

class Constructed : public virtual Type, public virtual Contained
{
public:
Constructed(const ContainerPtr& container, const std::string& name);
void destroy() final;
};

// ----------------------------------------------------------------------
// ClassDecl
// ----------------------------------------------------------------------

class ClassDecl final : public virtual Constructed, public std::enable_shared_from_this<ClassDecl>
class ClassDecl final : public Contained, public Type, public std::enable_shared_from_this<ClassDecl>
{
public:
ClassDecl(const ContainerPtr& container, const std::string& name);
Expand All @@ -583,10 +572,10 @@ namespace Slice
// Note: For the purpose of this parser, a class definition is not
// considered to be a type, but a class declaration is. And each class
// definition has at least one class declaration (but not vice versa),
// so if you need the class as a "constructed type", use the
// so if you need the class as a "type", use the
// declaration() operation to navigate to the class declaration.
//
class ClassDef final : public virtual Container, public virtual Contained
class ClassDef final : public Container, public Contained
{
public:
ClassDef(const ContainerPtr& container, const std::string& name, int id, ClassDefPtr base);
Expand Down Expand Up @@ -628,7 +617,7 @@ namespace Slice
// InterfaceDecl
// ----------------------------------------------------------------------

class InterfaceDecl final : public virtual Constructed, public std::enable_shared_from_this<InterfaceDecl>
class InterfaceDecl final : public Contained, public Type, public std::enable_shared_from_this<InterfaceDecl>
{
public:
InterfaceDecl(const ContainerPtr& container, const std::string& name);
Expand Down Expand Up @@ -667,7 +656,7 @@ namespace Slice
// Operation
// ----------------------------------------------------------------------

class Operation final : public virtual Contained, public virtual Container
class Operation final : public Container, public Contained
{
public:
//
Expand Down Expand Up @@ -706,6 +695,7 @@ namespace Slice
[[nodiscard]] std::optional<FormatType> format() const;
[[nodiscard]] std::string kindOf() const final;
void visit(ParserVisitor* visitor) final;
void destroy() final;

private:
TypePtr _returnType;
Expand All @@ -723,10 +713,10 @@ namespace Slice
// Note: For the purpose of this parser, an interface definition is not
// considered to be a type, but an interface declaration is. And each interface
// definition has at least one interface declaration (but not vice versa),
// so if you need the interface as a "constructed type", use the
// so if you need the interface as a "type", use the
// declaration() function to navigate to the interface declaration.
//
class InterfaceDef final : public virtual Container, public virtual Contained
class InterfaceDef final : public Container, public Contained
{
public:
InterfaceDef(const ContainerPtr& container, const std::string& name, InterfaceList bases);
Expand Down Expand Up @@ -768,9 +758,7 @@ namespace Slice
// ----------------------------------------------------------------------
// Exception
// ----------------------------------------------------------------------

// No inheritance from Constructed, as this is not a Type
class Exception final : public virtual Container, public virtual Contained
class Exception final : public Container, public Contained
{
public:
Exception(const ContainerPtr& container, const std::string& name, ExceptionPtr base);
Expand Down Expand Up @@ -801,7 +789,7 @@ namespace Slice
// Struct
// ----------------------------------------------------------------------

class Struct final : public virtual Container, public virtual Constructed
class Struct final : public Container, public Contained, public Type
{
public:
Struct(const ContainerPtr& container, const std::string& name);
Expand All @@ -820,13 +808,14 @@ namespace Slice
[[nodiscard]] bool isVariableLength() const final;
[[nodiscard]] std::string kindOf() const final;
void visit(ParserVisitor* visitor) final;
void destroy() final;
};

// ----------------------------------------------------------------------
// Sequence
// ----------------------------------------------------------------------

class Sequence final : public virtual Constructed, public std::enable_shared_from_this<Sequence>
class Sequence final : public Contained, public Type, public std::enable_shared_from_this<Sequence>
{
public:
Sequence(const ContainerPtr& container, const std::string& name, TypePtr type, MetadataList typeMetadata);
Expand All @@ -849,7 +838,7 @@ namespace Slice
// Dictionary
// ----------------------------------------------------------------------

class Dictionary final : public virtual Constructed, public std::enable_shared_from_this<Dictionary>
class Dictionary final : public Contained, public Type, public std::enable_shared_from_this<Dictionary>
{
public:
Dictionary(
Expand Down Expand Up @@ -885,11 +874,10 @@ namespace Slice
// Enum
// ----------------------------------------------------------------------

class Enum final : public virtual Container, public virtual Constructed
class Enum final : public Container, public Contained, public Type
{
public:
Enum(const ContainerPtr& container, const std::string& name);
void destroy() final;
EnumeratorPtr createEnumerator(const std::string& name, std::optional<int> explicitValue);
[[nodiscard]] bool hasExplicitValues() const;
[[nodiscard]] int minValue() const;
Expand All @@ -899,6 +887,7 @@ namespace Slice
[[nodiscard]] bool isVariableLength() const final;
[[nodiscard]] std::string kindOf() const final;
void visit(ParserVisitor* visitor) final;
void destroy() final;

private:
bool _hasExplicitValues{false};
Expand All @@ -911,7 +900,7 @@ namespace Slice
// Enumerator
// ----------------------------------------------------------------------

class Enumerator final : public virtual Contained
class Enumerator final : public Contained
{
public:
Enumerator(const ContainerPtr& container, const std::string& name, int value, bool hasExplicitValue);
Expand All @@ -921,6 +910,8 @@ namespace Slice
[[nodiscard]] bool hasExplicitValue() const;
[[nodiscard]] int value() const;

void visit(ParserVisitor* visitor) final;

private:
bool _hasExplicitValue;
int _value;
Expand All @@ -930,7 +921,7 @@ namespace Slice
// Const
// ----------------------------------------------------------------------

class Const final : public virtual Contained, public std::enable_shared_from_this<Const>
class Const final : public Contained, public std::enable_shared_from_this<Const>
{
public:
Const(
Expand Down Expand Up @@ -959,7 +950,7 @@ namespace Slice
// Parameter
// ----------------------------------------------------------------------

class Parameter final : public virtual Contained, public std::enable_shared_from_this<Parameter>
class Parameter final : public Contained, public std::enable_shared_from_this<Parameter>
{
public:
Parameter(
Expand Down Expand Up @@ -987,7 +978,7 @@ namespace Slice
// DataMember
// ----------------------------------------------------------------------

class DataMember final : public virtual Contained, public std::enable_shared_from_this<DataMember>
class DataMember final : public Contained, public std::enable_shared_from_this<DataMember>
{
public:
DataMember(
Expand Down Expand Up @@ -1018,11 +1009,12 @@ namespace Slice
// Unit
// ----------------------------------------------------------------------

class Unit final : public virtual Container
class Unit final : public Container
{
public:
static UnitPtr
createUnit(std::string languageName, bool all, const StringList& defaultFileMetadata = StringList());
Unit(std::string languageName, bool all, MetadataList defaultFileMetadata);

[[nodiscard]] std::string languageName() const;

Expand Down Expand Up @@ -1066,8 +1058,9 @@ namespace Slice

int parse(const std::string& filename, FILE* file, bool debugMode);

void destroy() final;
void visit(ParserVisitor* visitor) final;
void destroy();
void visit(ParserVisitor* visitor);
[[nodiscard]] UnitPtr unit() final;

// Not const, as builtins are created on the fly. (Lazy initialization.)
BuiltinPtr createBuiltin(Builtin::Kind kind);
Expand All @@ -1076,8 +1069,6 @@ namespace Slice
[[nodiscard]] std::set<std::string> getTopLevelModules(const std::string& file) const;

private:
Unit(std::string languageName, bool all, MetadataList defaultFileMetadata);

void pushDefinitionContext();
void popDefinitionContext();

Expand Down
6 changes: 3 additions & 3 deletions cpp/src/ice2slice/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,9 +734,9 @@ Slice::Gen::TypesVisitor::visitConst(const ConstPtr& p)
}
else
{
auto constructed = dynamic_pointer_cast<Constructed>(p->type());
assert(constructed);
typeString = constructed->scoped();
auto contained = dynamic_pointer_cast<Contained>(p->type());
assert(contained);
typeString = contained->scoped();
}

Output& out = getOutput(p);
Expand Down
3 changes: 1 addition & 2 deletions cpp/src/slice2cpp/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,10 +423,9 @@ namespace
if (dynamic_pointer_cast<ClassDef>(p) || dynamic_pointer_cast<ClassDecl>(p) ||
dynamic_pointer_cast<Struct>(p) || dynamic_pointer_cast<Slice::Exception>(p))
{
UnitPtr unt = p->container()->unit();
string file = p->file();
assert(!file.empty());
DefinitionContextPtr dc = unt->findDefinitionContext(file);
DefinitionContextPtr dc = p->unit()->findDefinitionContext(file);
assert(dc);

// TODO: why do we ignore all instances of this metadata except the first?
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/slice2cs/CsUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ Slice::CsGenerator::writeMarshalUnmarshalCode(
return;
}

assert(dynamic_pointer_cast<Constructed>(type));
assert(dynamic_pointer_cast<Contained>(type));
string helperName;
DictionaryPtr d = dynamic_pointer_cast<Dictionary>(type);
if (d)
Expand Down
Loading