-
Notifications
You must be signed in to change notification settings - Fork 1
Create classes for syntax nodes, variables, assignment #1
Changes from 3 commits
996d119
db8b9db
0c8197b
66eaecd
0c1ccb7
80ed405
fd4c7d5
7686fc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import SyntaxNode from './syntaxnode'; | ||
import Variable from './variable'; | ||
import Reference from './reference'; | ||
|
||
export default class Assignment implements SyntaxNode { | ||
private lhs: Reference; | ||
private rhs: SyntaxNode; | ||
|
||
constructor(lhs: Reference, rhs: SyntaxNode) { | ||
this.lhs = lhs; | ||
this.rhs = rhs; | ||
} | ||
|
||
public dependencies(): Set<Variable> { | ||
return this.rhs.dependencies(); | ||
} | ||
|
||
public source(): string { | ||
return `${this.lhs.source()} = ${this.rhs.source()};`; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,8 +22,8 @@ | |
* THE SOFTWARE. | ||
*/ | ||
|
||
import { Variable } from './variable'; | ||
|
||
export namespace Calder { | ||
export class Variable { } | ||
} | ||
export * from './variable'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason for removing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding stuff to namespaces is nice but it makes it kind of weird importing from another file, because of your class adds to a namespace, the file is no longer a module you can import |
||
export * from './function'; | ||
export * from './reference'; | ||
export * from './shader'; | ||
export * from './qualifier'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import Variable from './variable'; | ||
import SyntaxNode from './syntaxnode'; | ||
|
||
export default class Function implements SyntaxNode { | ||
public readonly name: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhh TypeScript has C#'s |
||
private statements: SyntaxNode[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you change this type to be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. I could make two subtypes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually I realize the interfaces aren't exactly the same, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
constructor(name: string, statements: SyntaxNode[] = []) { | ||
this.name = name; | ||
this.statements = statements; | ||
} | ||
|
||
public dependencies(): Set<Variable> { | ||
return this.statements.reduce( | ||
(union, statement) => new Set([...union, ...statement.dependencies()]), | ||
new Set()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bit of a style nitpick here, but could you write the code this way instead? return this.statements.reduce((union, statement) =>
new Set([...union, ...statement.dependencies()]),
new Set()
); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reasoning? imo it looks a bit weird visually pairing the return value of the lambda with the next parameter in the function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
public source(): string { | ||
// TODO: allow more than void() | ||
return `void ${this.name}() {\n` + | ||
this.statements | ||
.map(statement => statement.source()) | ||
.join('\n') + | ||
'\n}'; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
enum Qualifier { | ||
Attribute = 'attribute', | ||
Uniform = 'uniform', | ||
Varying = 'varying', | ||
Const = 'const', | ||
In = 'in', | ||
Out = 'out', | ||
InOut = 'inout' | ||
} | ||
|
||
export default Qualifier; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rip you cant There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sad ts reaccs only |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import Variable from './variable'; | ||
import SyntaxNode from './syntaxnode'; | ||
|
||
export default class Reference implements SyntaxNode { | ||
private variable: Variable; | ||
|
||
constructor(variable: Variable) { | ||
this.variable = variable; | ||
} | ||
|
||
public dependencies(): Set<Variable> { | ||
return new Set([this.variable]); | ||
} | ||
|
||
public source(): string { | ||
return this.variable.name; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import Variable from './variable'; | ||
import Function from './function'; | ||
|
||
export default class Shader { | ||
public main: Function; | ||
|
||
constructor(main: Function = new Function('main')) { | ||
this.main = main; | ||
} | ||
|
||
public source(): string { | ||
return `${this.header()}\n${this.main.source()}`; | ||
} | ||
|
||
private header(): string { | ||
return [...this.main.dependencies()] | ||
.map(dependency => dependency.declaration()) | ||
.join('\n'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import Variable from './variable'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So here, you're importing all of the code from in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update for reference later: we moved out |
||
|
||
export default interface SyntaxNode { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice interface! 💯 |
||
dependencies(): Set<Variable>; | ||
source(): string; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,18 @@ | ||
/** | ||
* Calder - A WebGL library for writing WebGL shaders | ||
* | ||
* Copyright (c) 2017 Paul Bardea, Abhishek Madan, Andrew McBurney, Dave Pagurek | ||
* | ||
* Permission is hereby granted, free of charge, to any person obtaining a copy | ||
* of this software and associated documentation files (the "Software"), to deal | ||
* in the Software without restriction, including without limitation the rights | ||
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
* copies of the Software, and to permit persons to whom the Software is | ||
* furnished to do so, subject to the following conditions: | ||
* | ||
* The above copyright notice and this permission notice shall be included in | ||
* all copies or substantial portions of the Software. | ||
* | ||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
* THE SOFTWARE. | ||
*/ | ||
import Qualifier from './qualifier'; | ||
|
||
export class Variable { | ||
export default class Variable { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will we have a separate type for local variables, or will we just extend this type to handle global and local variables? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could maybe extend this one, but I haven't thought through too hard to see what differences there would need to be |
||
public readonly name: string; | ||
public readonly qualifier: Qualifier; | ||
public readonly kind: string; | ||
|
||
// TODO: typecheck kind | ||
constructor(qualifier: Qualifier, kind: string, name: string) { | ||
this.kind = kind; | ||
this.qualifier = qualifier; | ||
this.name = name; | ||
} | ||
|
||
public declaration(): string { | ||
return `${this.qualifier} ${this.kind} ${this.name};`; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import { expect } from 'chai'; | ||
import * as cgl from './calder'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for clarification, is this is how people will interface with our library in the future? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll probably want to make some builders later, but for now, yeah |
||
|
||
describe('Shader', () => { | ||
describe('source', () => { | ||
it('generates the expected source code', () => { | ||
const glPosition = new cgl.Variable(cgl.Qualifier.Out, 'vec4', 'gl_Position'); | ||
const vertexPosition = new cgl.Variable(cgl.Qualifier.Attribute, 'vec4', 'vertexPosition'); | ||
const shader = new cgl.Shader(new cgl.Function('main', [ | ||
new cgl.Assignment( | ||
new cgl.Reference(glPosition), | ||
new cgl.Reference(vertexPosition))])); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a lot of braces here - and while I love lisp (emacs is bae 😍), I think this would have improved readability with some of the braces lined up with the indents: const shader = new cgl.Shader(
new cgl.Function('main', [
new cgl.Assignment(
new cgl.Reference(glPosition),
new cgl.Reference(vertexPosition)
)
])
); or even more concisely (although I'm a bigger fan of the first): const shader = new cgl.Shader(
new cgl.Function('main', [
new cgl.Assignment(new cgl.Reference(glPosition), new cgl.Reference(vertexPosition))
])
); |
||
|
||
const source = shader.source(); | ||
|
||
// TODO: recognize gl_Position as a default that doesn't need declaring | ||
const expected = ` | ||
attribute vec4 vertexPosition; | ||
out vec4 gl_Position; | ||
|
||
void main() { | ||
gl_Position = aVertexPosition; | ||
}`; | ||
|
||
expect(source).to.equalIgnoreSpaces(expected); | ||
}); | ||
}) | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,9 @@ | |
"module": "es6", | ||
"moduleResolution": "node", | ||
"experimentalDecorators": true, | ||
"noImplicitAny": true | ||
"noImplicitAny": true, | ||
"strictNullChecks": true, | ||
"lib": ["es5", "es6", "dom"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice 👍 |
||
}, | ||
"exclude": ["node_modules"] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to reference
this
explicitly, or is TypeScript smart enough to figure out you're referring to the particular instance in question, given that it's an instance method for the class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need
this
explicitlyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sad js reaccs only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢