Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(4096)

Issue 10247043: Use new symbol table initization for vertex shader. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 11 months ago by Alan Leung Chromium
Modified:
8 years, 11 months ago
Reviewers:
Alok Priyadarshi
CC:
angleproject-review_googlegroups.com
Base URL:
https://angleproject.googlecode.com/svn/trunk
Visibility:
Public.

Description

Use new symbol table initization for vertex shader. BUG= R=alokp@chromium.org Committed: https://code.google.com/p/angleproject/source/detail?r=2426

Patch Set 1 #

Patch Set 2 : remove pool allocate changes #

Total comments: 12

Patch Set 3 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1849 lines, -1801 lines) Patch
M src/compiler/Compiler.cpp View 1 2 1 chunk +14 lines, -1 line 0 comments Download
M src/compiler/Initialize.cpp View 2 chunks +0 lines, -26 lines 0 comments Download
M src/compiler/builtin_symbol_table.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M src/compiler/builtin_symbol_table.cpp View 1 2 3 chunks +11 lines, -1 line 0 comments Download
M src/compiler/builtin_symbols.json View 1 2 1 chunk +1790 lines, -1750 lines 0 comments Download
M src/compiler/generate_builtin_symbol_table.py View 1 2 2 chunks +32 lines, -22 lines 0 comments Download

Messages

Total messages: 5
Alan Leung Chromium
8 years, 11 months ago (2013-06-12 23:08:28 UTC) #1
Alan Leung Chromium
Moved the vertex shader builtin to use the new initialization system.
8 years, 11 months ago (2013-06-12 23:09:12 UTC) #2
Alok Priyadarshi
lgtm https://codereview.appspot.com/10247043/diff/2001/src/compiler/Compiler.cpp File src/compiler/Compiler.cpp (right): https://codereview.appspot.com/10247043/diff/2001/src/compiler/Compiler.cpp#newcode133 src/compiler/Compiler.cpp:133: please delete this line. https://codereview.appspot.com/10247043/diff/2001/src/compiler/builtin_symbol_table.cpp File src/compiler/builtin_symbol_table.cpp (right): ...
8 years, 11 months ago (2013-06-13 17:20:43 UTC) #3
Alan Leung Chromium
Committed patchset #3 manually as r2426 (presubmit successful).
8 years, 11 months ago (2013-06-13 19:36:40 UTC) #4
Alan Leung Chromium
8 years, 11 months ago (2013-06-13 19:36:55 UTC) #5
Message was sent while issue was closed.
https://codereview.appspot.com/10247043/diff/2001/src/compiler/Compiler.cpp
File src/compiler/Compiler.cpp (right):

https://codereview.appspot.com/10247043/diff/2001/src/compiler/Compiler.cpp#n...
src/compiler/Compiler.cpp:133: 
On 2013/06/13 17:20:43, Alok Priyadarshi wrote:
> please delete this line.

Done.

https://codereview.appspot.com/10247043/diff/2001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.cpp (right):

https://codereview.appspot.com/10247043/diff/2001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.cpp:283: void
InsertBuiltInFunctionsVertex(const ShBuiltInResources& resources, TSymbolTable *
t) {
On 2013/06/13 17:20:43, Alok Priyadarshi wrote:
> nit: get rid of space between TSymbolTable and '*'.

Done.

https://codereview.appspot.com/10247043/diff/2001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.h (right):

https://codereview.appspot.com/10247043/diff/2001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:14: extern void
InsertBuiltInFunctionsVertex(const ShBuiltInResources& resources, TSymbolTable *
t);
On 2013/06/13 17:20:43, Alok Priyadarshi wrote:
> here too.

Done.

https://codereview.appspot.com/10247043/diff/2001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbols.json (right):

https://codereview.appspot.com/10247043/diff/2001/src/compiler/builtin_symbol...
src/compiler/builtin_symbols.json:1778: {
On 2013/06/13 17:20:43, Alok Priyadarshi wrote:
> nit: indent the array entries:
> "vertex": [
>   {
>     "name": "foo",
>     ...
>   },
>   {
>   },
>   ...
> ]

Done.

https://codereview.appspot.com/10247043/diff/2001/src/compiler/generate_built...
File src/compiler/generate_builtin_symbol_table.py (right):

https://codereview.appspot.com/10247043/diff/2001/src/compiler/generate_built...
src/compiler/generate_builtin_symbol_table.py:67: # A function that parse the
JSON representation of a list of builtin functions.
On 2013/06/13 17:20:43, Alok Priyadarshi wrote:
> parse -> parses

Done.

https://codereview.appspot.com/10247043/diff/2001/src/compiler/generate_built...
src/compiler/generate_builtin_symbol_table.py:69: for func in builtin[name]:
On 2013/06/13 17:20:43, Alok Priyadarshi wrote:
> nit: 4-space indent

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b