Skip to content

Commit

Permalink
schema2cpp: abandon inheritance and use a simple template ctor (Polym…
Browse files Browse the repository at this point in the history 
…erLabs#4059)

* schema2cpp: abandon inheritance and use a simple template ctor

Using inheritance to mirror the type lattice Just Doesn't Work(TM) in C++
because even completely abstract classes aren't really interfaces. So,
if two unrelated base classes have a common field, this introduces
ambiguity even if they are just declaring an accessor method and don't
have the field actually defined. Sigh.

So, no more inheritance. All entity classes are stand-alone, but with a
template constructor which allows slicable entities to be implicitly
converted at call sites, which is reasonable (and actually a lot easier
to implement).

* Removed TODO, added comment
  • Loading branch information
@mykmartin
mykmartin authored Nov 13, 2019
1 parent cfd32ad commit 7f5c94c
Show file tree
Hide file tree
Showing 9 changed files with 198 additions and 311 deletions.
15 changes: 7 additions & 8 deletions src/tools/schema2base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import {Runtime} from '../runtime/runtime.js';
import {SchemaGraph, SchemaNode} from './schema2graph.js';

export interface ClassGenerator {
addField(field: string, typeChar: string, inherited: boolean);
addReference(field: string, inherited: boolean, refName: string);
addField(field: string, typeChar: string);
addReference(field: string, refName: string);
generate(fieldCount: number): string;
}

Expand Down Expand Up @@ -78,26 +78,25 @@ export abstract class Schema2Base {
const fields = Object.entries(node.schema.fields);

for (const [field, descriptor] of fields) {
const inherited = !node.extras.includes(field);
switch (this.typeSummary(descriptor)) {
case 'schema-primitive:Text':
generator.addField(field, 'T', inherited);
generator.addField(field, 'T');
break;

case 'schema-primitive:URL':
generator.addField(field, 'U', inherited);
generator.addField(field, 'U');
break;

case 'schema-primitive:Number':
generator.addField(field, 'N', inherited);
generator.addField(field, 'N');
break;

case 'schema-primitive:Boolean':
generator.addField(field, 'B', inherited);
generator.addField(field, 'B');
break;

case 'schema-reference':
generator.addReference(field, inherited, node.refs.get(field).name);
generator.addReference(field, node.refs.get(field).name);
break;

default:
Expand Down
72 changes: 35 additions & 37 deletions src/tools/schema2cpp.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export class Schema2Cpp extends Schema2Base {
}

getClassGenerator(node: SchemaNode): ClassGenerator {
return new CppGenerator(node, this.scope.replace('.', '::'));
return new CppGenerator(node, this.scope.replace(/\./g, '::'));
}
}

Expand All @@ -65,6 +65,7 @@ function fixName(field: string): string {
class CppGenerator implements ClassGenerator {
fields: string[] = [];
api: string[] = [];
ctor: string[] = [];
clone: string[] = [];
hash: string[] = [];
equals: string[] = [];
Expand All @@ -75,23 +76,23 @@ class CppGenerator implements ClassGenerator {

constructor(readonly node: SchemaNode, readonly namespace: string) {}

addField(field: string, typeChar: string, inherited: boolean) {
addField(field: string, typeChar: string) {
const {type, defaultVal, isString} = typeMap[typeChar];
const [r1, r2] = isString ? ['const ', '&'] : ['', ''];
const fixed = fixName(field);
const valid = `${field}_valid_`;

// Fields inherited from a base class don't need member declarations or API methods in this one.
if (!inherited) {
this.fields.push(`${type} ${field}_${defaultVal};`,
`bool ${valid} = false;`,
``);

this.api.push(`${r1}${type}${r2} ${fixName(field)}() const { return ${field}_; }`,
`void set_${field}(${r1}${type}${r2} value) { ${field}_ = value; ${valid} = true; }`,
`void clear_${field}() { ${field}_${defaultVal}; ${valid} = false; }`,
`bool has_${field}() const { return ${valid}; }`,
``);
}
this.fields.push(`${type} ${field}_${defaultVal};`,
`bool ${valid} = false;`,
``);

this.api.push(`${r1}${type}${r2} ${fixed}() const { return ${field}_; }`,
`void set_${field}(${r1}${type}${r2} value) { ${field}_ = value; ${valid} = true; }`,
`void clear_${field}() { ${field}_${defaultVal}; ${valid} = false; }`,
`bool has_${field}() const { return ${valid}; }`,
``);

this.ctor.push(`${field}_(other.${fixed}()), ${valid}(other.has_${field}())`);

this.clone.push(`clone.${field}_ = entity.${field}_;`,
`clone.${valid} = entity.${valid};`);
Expand Down Expand Up @@ -126,16 +127,19 @@ class CppGenerator implements ClassGenerator {
` printer.add("${field}: ", entity.${field}_);`);
}

addReference(field: string, inherited: boolean, refName: string) {
addReference(field: string, refName: string) {
const type = `Ref<${refName}>`;
const fixed = fixName(field);

this.fields.push(`${type} ${field}_;`,
``);

this.api.push(`const ${type}& ${fixName(field)}() const { return ${field}_; }`,
this.api.push(`const ${type}& ${fixed}() const { return ${field}_; }`,
`void bind_${field}(const ${refName}& value) { internal::Accessor::bind(&${field}_, value); }`,
``);

this.ctor.push(`${field}_(other.${fixed}())`);

this.clone.push(`clone.${field}_ = entity.${field}_;`);

this.hash.push(`if (entity.${field}_._internal_id_ != "")`,
Expand All @@ -159,25 +163,17 @@ class CppGenerator implements ClassGenerator {
}

generate(fieldCount: number): string {
const {name, aliases, parents, children, sharesParent} = this.node;

let bases = '';
if (parents.length) {
// Add base classes. Use virtual inheritance if we know this schema shares a parent
// with another schema, and it also has descendant schemas. Note this means some
// false positives are possible, but that's not really a problem.
const spec = (sharesParent && children.length) ? 'virtual public' : 'public';
bases = ` : ${spec} ` + parents.map(p => p.name).join(`, ${spec} `);
} else {
// This class doesn't have any parents so it needs an id field (which
// will subsequently be inherited by any children of this class).
this.fields.push('std::string _internal_id_;');
}

// Use a virtual destructor for all schemas that participate in inheritance chains.
let dtor = '';
if (parents.length || children.length) {
dtor = `virtual ~${name}() {}\n`;
const {name, aliases} = this.node;

// Template constructor allows implicit type slicing from appropriately matching entities.
let templateCtor = '';
if (this.ctor.length) {
templateCtor = `\
template<typename T>
${name}(const T& other) :
${this.ctor.join(',\n ')}
{}
`;
}

// 'using' declarations for equivalent entity types.
Expand All @@ -197,14 +193,15 @@ class CppGenerator implements ClassGenerator {
namespace ${this.namespace} {
${aliasComment}
class ${name}${bases} {
class ${name} {
public:
// Entities must be copied with arcs::clone_entity(), which will exclude the internal id.
// Move operations are ok (and will include the internal id).
${name}() = default;
${name}(${name}&&) = default;
${name}& operator=(${name}&&) = default;
${dtor}
${templateCtor}
${this.api.join('\n ')}
// Equality ops compare internal ids and all data fields.
// Use arcs::fields_equal() to compare only the data fields.
Expand All @@ -225,6 +222,7 @@ protected:
${name}& operator=(const ${name}&) = default;
${this.fields.join('\n ')}
std::string _internal_id_;
static const int _FIELD_COUNT = ${fieldCount};
friend class Singleton<${name}>;
Expand Down
28 changes: 4 additions & 24 deletions src/tools/schema2graph.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,13 @@ export class SchemaNode {
parents: SchemaNode[] = null;
children: SchemaNode[] = null;

// True if any other schema has the same parent as this one. Used to set up virtual
// inheritance for C++ classes.
sharesParent = false;

// The list of field names that this schema has *in addition to* all of its ancestors.
extras: string[];

// Maps reference fields to the node for their contained schema. This is also used to
// ensure that nested schemas are generated before the references that rely on them.
refs = new Map<string, SchemaNode>();

constructor(schema: Schema, name: string) {
this.schema = schema;
this.aliases.push(name);
this.extras = Object.keys(schema.fields);
}
}

Expand All @@ -52,9 +44,8 @@ export class SchemaNode {
// For example, the schema '* {Text t, URL u}' is slicable to both '* {Text t}' and '* {URL u}'.
//
// The graph has a second set of edges via the refs field, which connects nodes whose schemas have
// references to other nodes which hold those references' nested schemas. These connections are used
// to ensure that classes are generated in the order needed to satisfy both their reference fields'
// type definitions and their inheritance heirarchies.
// references to other nodes which hold those references' nested schemas. These are used to ensure
// classes are generated in the order needed to satisfy their reference field type definitions.
export class SchemaGraph {
nodes: SchemaNode[] = [];
startNodes: SchemaNode[];
Expand All @@ -72,7 +63,7 @@ export class SchemaGraph {
// Both the second pass and the walk() method need to start from nodes with no parents.
this.startNodes = this.nodes.filter(n => !n.parents);

// Second pass to set up the class names, aliases and the parents, children and extras lists.
// Second pass to set up the class names, aliases, parents and children.
for (const node of this.startNodes) {
node.parents = [];
this.process(node);
Expand Down Expand Up @@ -140,20 +131,9 @@ export class SchemaGraph {
// children = (all descendants) - (descendants of descendants)
node.children = [...node.descendants].filter(x => !transitiveDescendants.has(x));

// Set up parent links on child nodes. If this node has multiple children, mark each
// of them as sharing a parent. This is used to set up virtual inheritance in C++.
// TODO: detect shared descendants across children for more accurate virtual inheritance
const sharesParent = node.children.length > 1;
const parentFields = Object.keys(node.schema.fields);
// Set up parent links on child nodes.
for (const child of node.children) {
child.parents.push(node);
child.sharesParent = child.sharesParent || sharesParent; // don't wipe previous true value

// Remove all of this node's field names (derived from the schema) from each child's extras
// list. This means that extras will end up naming only those fields that a schema has in
// addition to its entire ancestry tree.
child.extras = child.extras.filter(f => !parentFields.includes(f));

this.process(child);
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/tools/schema2kotlin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class KotlinGenerator implements ClassGenerator {

constructor(readonly node: SchemaNode) {}

addField(field: string, typeChar: string, inherited: boolean) {
addField(field: string, typeChar: string) {
const {type, decodeFn} = typeMap[typeChar];
const fixed = field + (keywords.includes(field) ? '_' : '');

Expand All @@ -82,7 +82,7 @@ class KotlinGenerator implements ClassGenerator {
this.encode.push(`${fixed}?.let { encoder.encode("${field}:${typeChar}", it) }`);
}

addReference(field: string, inherited: boolean, refName: string) {
addReference(field: string, refName: string) {
throw new Error('TODO: support reference types in kotlin');
}

Expand All @@ -103,20 +103,20 @@ ${withFields('data ')}class ${name}(${ withFields(`\n ${this.fields.join(',\n
override fun decodeEntity(encoded: String): ${name}? {
if (encoded.isEmpty()) return null
val decoder = StringDecoder(encoded)
val decoder = StringDecoder(encoded)
internalId = decoder.decodeText()
decoder.validate("|")
${withFields(` for (_i in 0 until ${fieldCount}) {
if (decoder.done()) break
if (decoder.done()) break
val name = decoder.upTo(":")
when (name) {
${this.decode.join('\n ')}
}
decoder.validate("|")
}
`)}
return this
}
Expand Down
22 changes: 0 additions & 22 deletions src/tools/tests/schema2cpp-test.ts
View file

This file was deleted.

Loading

0 comments on commit 7f5c94c

Please sign in to comment.