refactored SkSL VarDeclaration handling

refactored SkSL VarDeclaration handling
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2312233002

Committed: https://skia.googlesource.com/skia/+/14fe8cc16d2a98d5a96f818e4d0b7127ecad59e2

[ethannicholas, 2016-09-06 14:59:43]

Description was changed from

==========
refactored SkSL VarDeclaration handling
==========

to

==========
refactored SkSL VarDeclaration handling
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2312233002
==========

[ethannicholas, 2016-09-06 16:26:10]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 16:26:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 16:46:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 16:46:49]

Dry run: This issue passed the CQ dry run.

[ethannicholas, 2016-09-07 16:23:36]

ethannicholas@google.com changed reviewers:
+ benjaminwagner@google.com

[dogben, 2016-09-07 17:35:42]

https://codereview.chromium.org/2312233002/diff/1/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/2312233002/diff/1/src/sksl/SkSLParser.cpp#new...
src/sksl/SkSLParser.cpp:369: //currentVarSizes->push_back(nullptr);
Why is this commented out?

https://codereview.chromium.org/2312233002/diff/1/src/sksl/SkSLParser.cpp#new...
src/sksl/SkSLParser.cpp:420: value.release();
1. This should be reset, not release.
2. nit: move below currentVarSizes.clear() (or move both to the top of the loop)

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ast/SkSLASTVarDecl...
File src/sksl/ast/SkSLASTVarDeclaration.h (right):

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ast/SkSLASTVarDecl...
src/sksl/ast/SkSLASTVarDeclaration.h:34: for (size_t i = 0; i < fSizes.size();
i++) {
nit for (const auto& size : fSizes)

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ir/SkSLVarDeclarat...
File src/sksl/ir/SkSLVarDeclaration.h (right):

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ir/SkSLVarDeclarat...
src/sksl/ir/SkSLVarDeclaration.h:26: : fVar(std::move(var))
nit: std::move doesn't do anything here.

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ir/SkSLVarDeclarat...
src/sksl/ir/SkSLVarDeclaration.h:32: for (size_t i = 0; i < fSizes.size(); i++)
{
nit: for (const auto& size : fSizes)

[dogben, 2016-09-07 17:36:21]

(nits are ignorable)

[ethannicholas, 2016-09-07 19:46:38]

https://codereview.chromium.org/2312233002/diff/1/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/2312233002/diff/1/src/sksl/SkSLParser.cpp#new...
src/sksl/SkSLParser.cpp:369: //currentVarSizes->push_back(nullptr);
On 2016/09/07 17:35:42, Ben Wagner wrote:
> Why is this commented out?

That was an accident, oops.

https://codereview.chromium.org/2312233002/diff/1/src/sksl/SkSLParser.cpp#new...
src/sksl/SkSLParser.cpp:420: value.release();
On 2016/09/07 17:35:42, Ben Wagner wrote:
> 1. This should be reset, not release.
> 2. nit: move below currentVarSizes.clear() (or move both to the top of the
loop)

Done.

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ast/SkSLASTVarDecl...
File src/sksl/ast/SkSLASTVarDeclaration.h (right):

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ast/SkSLASTVarDecl...
src/sksl/ast/SkSLASTVarDeclaration.h:34: for (size_t i = 0; i < fSizes.size();
i++) {
On 2016/09/07 17:35:42, Ben Wagner wrote:
> nit for (const auto& size : fSizes)

Done.

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ir/SkSLVarDeclarat...
File src/sksl/ir/SkSLVarDeclaration.h (right):

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ir/SkSLVarDeclarat...
src/sksl/ir/SkSLVarDeclaration.h:26: : fVar(std::move(var))
On 2016/09/07 17:35:42, Ben Wagner wrote:
> nit: std::move doesn't do anything here.

Derp.

https://codereview.chromium.org/2312233002/diff/1/src/sksl/ir/SkSLVarDeclarat...
src/sksl/ir/SkSLVarDeclaration.h:32: for (size_t i = 0; i < fSizes.size(); i++)
{
On 2016/09/07 17:35:42, Ben Wagner wrote:
> nit: for (const auto& size : fSizes) 

Done.

[dogben, 2016-09-07 19:50:33]

lgtm

[ethannicholas, 2016-09-07 19:50:58]

The CQ bit was checked by ethannicholas@google.com

[commit-bot: I haz the power, 2016-09-07 19:51:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 20:37:18]

Description was changed from

==========
refactored SkSL VarDeclaration handling
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2312233002
==========

to

==========
refactored SkSL VarDeclaration handling
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2312233002

Committed:
https://skia.googlesource.com/skia/+/14fe8cc16d2a98d5a96f818e4d0b7127ecad59e2
==========

[commit-bot: I haz the power, 2016-09-07 20:37:19]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/14fe8cc16d2a98d5a96f818e4d0b7127ecad59e2

[ethannicholas, 2016-09-08 13:28:15]

Patchset #3 (id:40001) has been deleted