Create classes for syntax nodes, variables, assignment #1

davepagurek · 2017-10-20T23:03:42Z

General structure:

every node in a glsl program ends up as a SyntaxNode of some kind (e.g. variable reference, assignment, function)
each statement knows what it references so a header can be computed

Todo:

recognize builtins
fit local variables into the structure
make the test actually work (@AndrewMcBurney how do I reference the files in src?)
think harder about how scopes should work
think about how to write this in a less verbose way
might have to import chai-string for tests. not sure though because I haven't got that far yet

armcburney · 2017-10-21T16:25:03Z

src/assignment.ts

@@ -0,0 +1,21 @@
+import SyntaxNode from './syntaxnode';
+import { Variable } from './variable';


Can you remove the {} from the Variable one to be consistent, please?

Variable exports two things. You can only use the no bracket version of you export one thing with export default. I could split the two exports into two files but they're coupled enough that I thought they should go in one. Thoughts?

Ahh, that's why - I didn't realize there were multiple exports in variable.ts. I'd be a fan of splitting it up into separate files for different exports (SRP), but given that the code is coupled like you said, I'm okay with either 👍

armcburney · 2017-10-21T16:26:37Z

src/assignment.ts

+    }
+
+    public source(): string {
+        return `${this.lhs.source()} = ${this.rhs.source()};`;


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?

You need this explicitly

Sad js reaccs only

armcburney · 2017-10-21T16:29:18Z

src/calder.ts

-export namespace Calder {
-    export class Variable { }
-}
+export * from './variable';


Any reason for removing the namespace?

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

armcburney · 2017-10-21T16:33:47Z

src/function.ts

+    public dependencies(): Set<Variable> {
+        return this.statements.reduce(
+            (union, statement) => new Set([...union, ...statement.dependencies()]),
+            new Set());


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() );

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

I like putting the lambda parameters on the same line as the function call reduce. Just seems like a convention for JavaScript arrow functions: 1, 2, 3.

armcburney · 2017-10-21T16:34:40Z

src/function.ts

+import SyntaxNode from './syntaxnode';
+
+export default class Function implements SyntaxNode {
+    public readonly name: string;


Ahhh TypeScript has C#'s readonly 😍

armcburney · 2017-10-21T16:39:53Z

src/syntaxnode.ts

@@ -0,0 +1,6 @@
+import Variable from './variable';


So here, you're importing all of the code from in variable.ts, right? This includes the exported enum Qualifier. Should you do import import { Variable } from './variable'; instead?

update for reference later: we moved out Qualifier so Variable is a default export now

armcburney · 2017-10-21T16:40:09Z

src/syntaxnode.ts

@@ -0,0 +1,6 @@
+import Variable from './variable';
+
+export default interface SyntaxNode {


Nice interface! 💯

armcburney · 2017-10-21T16:44:13Z

src/variable.ts

- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
+export enum Qualifier {


I know this is coupled to Variable, but I'd be a fan of splitting this up into another file, since we're exporting it as part of variable.ts's public interface. It could technically be used in a context outside of a Variable instance, despite being tightly coupled to Variable. Seems like a code smell. 👃

I'd recommend splitting this up into two files, and requiring Qualifier in Variable, or making this enum private to Variable. I don't know 100% what we'll want to use Qualifier for in the future (outside of the Variable class), so you're probably in the best place to make the judgment call.

davepagurek · 2017-10-21T16:49:03Z

src/qualifier.ts

+    InOut = 'inout'
+}
+
+export default Qualifier;


rip you cant export default enum in TS but you can do this for some reason: microsoft/TypeScript#3320

Sad ts reaccs only

armcburney · 2017-10-21T16:51:54Z

test/shader.spec.ts

@@ -0,0 +1,28 @@
+import { expect } from 'chai';
+import * as cgl from './calder';


Just for clarification, is this is how people will interface with our library in the future?

We'll probably want to make some builders later, but for now, yeah

armcburney · 2017-10-21T16:56:22Z

test/shader.spec.ts

+            const shader = new cgl.Shader(new cgl.Function('main', [
+                new cgl.Assignment(
+                    new cgl.Reference(glPosition),
+                    new cgl.Reference(vertexPosition))]));


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)) ]) );

armcburney · 2017-10-21T16:59:20Z

tsconfig.json

-    "noImplicitAny": true
+    "noImplicitAny": true,
+    "strictNullChecks": true,
+    "lib": ["es5", "es6", "dom"]


armcburney · 2017-10-21T17:27:43Z

test/shader.spec.ts

-                new cgl.Assignment(
-                    new cgl.Reference(glPosition),
-                    new cgl.Reference(vertexPosition))]));
+            const shader = new cgl.Shader(


Nice commit name 😂

abhimadan · 2017-10-22T05:45:02Z

src/function.ts

+
+export default class Function implements SyntaxNode {
+    public readonly name: string;
+    private statements: SyntaxNode[];


Can you change this type to be a Statement[]? You'd need to create a new SyntaxNode type, but it would avoid being able to do wacky stuff like having just a list of References. If you feel that this might be better addressed in a future PR, can you add a TODO?

Makes sense. I could make two subtypes Statement and Expression that for now are the exact same as the super for now maybe?

actually I realize the interfaces aren't exactly the same, Expression has a return type. Statement is a class now that just adds a semicolon to a SyntaxNode. I took the semicolon out of the Assignment class because I realize you can have those in the middle of statements too.

Actually Statement probably shouldn't JUST be a semicolon because ifs and whiles also are statements. Going to make that a separate issue for now though

calder-gl/calder#9

abhimadan · 2017-10-22T05:48:50Z

src/variable.ts


-export class Variable {
+export default class Variable {


Will we have a separate type for local variables, or will we just extend this type to handle global and local variables?

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

davepagurek · 2017-10-22T14:24:20Z

Ok tests work now!

armcburney · 2017-10-22T19:51:13Z

gulpfile.babel.js

 import typescript from "gulp-typescript";

-const src = ["./src/**/*.ts", "./test/**/*.ts"];
+const src = ["./src/**/*.ts"];
+const test =  ["./test/**/*.js"]


armcburney · 2017-10-22T19:54:31Z

src/type.ts

+    Mat3 = 'mat3',
+    Mat4 = 'mat4',
+    Sampler2D = 'sampler2D',
+    SamplerCube = 'SamplerCube'


Should this be SamplerCube = 'samplerCube'?

armcburney · 2017-10-22T19:58:40Z

test/test.spec.ts

@@ -1,10 +0,0 @@
-import { expect } from 'chai';


Rip test/test.spec.ts 💀 October 2017 - October 2017

armcburney

Tests pass, comments addressed. LGTM

abhimadan

LGTM!

davepagurek added 2 commits October 20, 2017 19:00

Create classes for syntax nodes, variables, assignment

996d119

Add brackets to function

db8b9db

armcburney self-requested a review October 21, 2017 02:26

armcburney reviewed Oct 21, 2017

View reviewed changes

src/syntaxnode.ts

@@ -0,0 +1,6 @@

import Variable from './variable';

export default interface SyntaxNode {

Copy link

Member

armcburney Oct 21, 2017

Choose a reason for hiding this comment

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

Nice interface! 💯

armcburney reviewed Oct 21, 2017

View reviewed changes

Move Qualifier into its own file

0c8197b

davepagurek commented Oct 21, 2017

View reviewed changes

armcburney reviewed Oct 21, 2017

View reviewed changes

tsconfig.json

"noImplicitAny": true

"noImplicitAny": true,

"strictNullChecks": true,

"lib": ["es5", "es6", "dom"]

Copy link

Member

armcburney Oct 21, 2017

Choose a reason for hiding this comment

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

Nice 👍

davepagurek added 2 commits October 21, 2017 13:08

Lambda syntax

66eaecd

Move brackets to feel less like lisp

0c1ccb7

armcburney reviewed Oct 21, 2017

View reviewed changes

abhimadan reviewed Oct 22, 2017

View reviewed changes

davepagurek added 2 commits October 22, 2017 09:50

Make tests use es6

80ed405

Add types, separate statement and expression

fd4c7d5

armcburney reviewed Oct 22, 2017

View reviewed changes

Fix capitalization

7686fc1

armcburney reviewed Oct 22, 2017

View reviewed changes

armcburney approved these changes Oct 22, 2017

View reviewed changes

abhimadan approved these changes Oct 22, 2017

View reviewed changes

davepagurek merged commit d007bb1 into master Oct 23, 2017

davepagurek deleted the initial-setup branch October 23, 2017 01:46

		@@ -0,0 +1,21 @@
		import SyntaxNode from './syntaxnode';
		import { Variable } from './variable';

		@@ -0,0 +1,6 @@
		import Variable from './variable';

		export default interface SyntaxNode {

		@@ -0,0 +1,28 @@
		import { expect } from 'chai';
		import * as cgl from './calder';

Create classes for syntax nodes, variables, assignment #1

Create classes for syntax nodes, variables, assignment #1

Conversation

davepagurek commented Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armcburney Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armcburney Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armcburney left a comment

Choose a reason for hiding this comment

abhimadan left a comment

Choose a reason for hiding this comment

davepagurek commented Oct 20, 2017 •

edited

Loading

armcburney Oct 21, 2017 •

edited

Loading

armcburney Oct 21, 2017 •

edited

Loading