Express (GLSL expression, possibly known value) pairs as a class

Express (GLSL expression, possibly known value) pairs as a class

Express (GLSL expression, possibly known value) pairs as a class
instead of two variables Introduces GrGLSLExpr<N> to encapsulate
the expression and possibly constant-folded value of the expression.

This simplifies passing of the expressions to functions.

Changes the shaders with following patterns:
        { // Stage 0: Linear Gradient
        vec4 colorTemp = mix(uGradientStartColor_Stage0, uGradientEndColor_Stage0, clamp(vMatrixCoord_Stage0.x, 0.0, 1
        colorTemp.rgb *= colorTemp.a;
-       output_Stage0 = vec4((vColor) * (colorTemp));
+       output_Stage0 = (vColor * colorTemp);
+       }

Previously the vector cast was always added if constant folding was
effective, regardless of the term dimensions. Now the vector upcast is
not inserted in places where it is not needed, ie. when the binary
operator term is of the target dimension.

Also, some parentheses can be omitted. It is assumed that
GrGLSLExpr<N>("string") constructors construct a simple expression or
parenthesized expression.

Otherwise the shader code remains identical.

Committed: http://code.google.com/p/skia/source/detail?r=11690

[Kimmo Kinnunen, 2013-10-01 12:58:12]

Sorry for the "refactor" type commit without proper functionality change, but in
simplified https://codereview.chromium.org/25023003/

[bsalomon, 2013-10-02 13:20:25]

Kimmo, I'm so glad you did this! I've had a change like this on the back of my
mind for a long time and probably wouldn't have gotten around to it for a long
time.

Sorry for delayed review. I was out yesterday. I've looked at the GrGLSL files
so far and not the users of it, yet. I have to leave for a Dr's appt and will
get to the rest afterwards.

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL.h#newco...
src/gpu/gl/GrGLSL.h:74: inline const char* GrGLSLTypeString(GrSLType t) {
probably want both static and inline here, right?

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL.h#newco...
src/gpu/gl/GrGLSL.h:118: SkASSERT(!isValid());
this->isValid()

several more below

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL.h#newco...
src/gpu/gl/GrGLSL.h:121: explicit GrGLSLExpr(int v) {

Comment that this replicates the int to all components?

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL.h#newco...
src/gpu/gl/GrGLSL.h:128: fExpr.appendf(castIntStr(), v);
this->castIntStr()

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL.h#newco...
src/gpu/gl/GrGLSL.h:154: /** Given an expression of vecM, cast it to vecN. */
Document what happens if N > M?

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL.h#newco...
src/gpu/gl/GrGLSL.h:161: template<int M>
Maybe a comment here about how a vecN and vecM are combined?

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL_impl.h
File src/gpu/gl/GrGLSL_impl.h (right):

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL_impl.h#...
src/gpu/gl/GrGLSL_impl.h:13: inline const char* GrGLSLExpr<4>::zerosStr() {
should zeros and ones be static?

[bsalomon, 2013-10-02 15:18:03]

https://codereview.chromium.org/25048002/diff/10001/src/effects/SkArithmeticM...
File src/effects/SkArithmeticMode.cpp (right):

https://codereview.chromium.org/25048002/diff/10001/src/effects/SkArithmeticM...
src/effects/SkArithmeticMode.cpp:371: builder->fsCodeAppendf("\t\tconst vec4 src
= %s;\n", GrGLSLExpr<4>(1).c_str());
Hmm.. This feels a little bit less readable.

I'd say we should keep the old func or have a globally accessible const
GrGLExpr<4> initialized to all 1s:

GrGLExpr<4>::Ones()

Also, what do you think of:
typedef GrGLSLExpr<4> GrGLSLExpr4?

I think it might make the code a little more readable.

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLProgram.cp...
src/gpu/gl/GrGLProgram.cpp:297: coeff = GrGLSLExpr<4>::VectorCast(1 -
inColor.a());
This line looks a bit odd to me. Does the 1 get casted to a GrGLSLExpr<1>?

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

https://codereview.chromium.org/25048002/diff/10001/src/gpu/gl/GrGLSL.h#newco...
src/gpu/gl/GrGLSL.h:168: /**
Maybe we ought to go ahead and have r() g() and b()?

[Kimmo Kinnunen, 2013-10-08 12:18:28]

Thanks. New version + some discussion.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/effects/SkArit...
File src/effects/SkArithmeticMode.cpp (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/effects/SkArit...
src/effects/SkArithmeticMode.cpp:371: builder->fsCodeAppendf("\t\tconst vec4 src
= %s;\n", GrGLSLExpr<4>(1).c_str());
On 2013/10/02 15:18:04, bsalomon wrote:
> Hmm.. This feels a little bit less readable.

I fixed this and the other similar instance by just not using the func, since
these didn't need it ever anyway. Would that be ok?

> I'd say we should keep the old func or have a globally accessible const
> GrGLExpr<4> initialized to all 1s:
> 
> GrGLExpr<4>::Ones()

The Ones constructor is not nearly as useful as it initially appears. If you
want to see the pattern, look at the first patch.
The problem with it is that you can't do all the equivalent calculation, namely
the "1 minus alpha" case. The alpha accessor returns instance of a type that can
not be referred to while preserving idiomatic syntax. The "1" is useful when it
represents ones of all dimensions, though I'm not sure I'm able to argue/talk
about this in any type-theoretic way.

Current "unreadable" usecase is solely "return string of vec4(1)". All the
current cases can be removed when inputColor is being passed as an expression.
Most of the future cases that I can think of will need that string in context of
big glsl statement blocks, where the author has written the var4 statement too.
In these cases the 1,1,1,1 string can and should be written manually. Eg.
there's no need to use the possible variability that the dimension variable
would provide.

Then the "unreadable" concern becomes issue of which of the following is
idiomatic:

GrGLSLExpr<4> coeff(1);  // A
GrGLSLExpr<4> coeff = GrGLSLExpr<4>::Ones; // B

As far as readibility goes, I think the case A is more readable as there's less
to look at. Your call.

If you want the Ones constructor:
Other working option that I can think of is the pattern where "One" constructor
would return a new type GrGLSLExprOnes which then would be convertible to Expr
of any dimension. I didn't go this route as it seemed to add template bloat.

> Also, what do you think of:
> typedef GrGLSLExpr<4> GrGLSLExpr4?
> I think it might make the code a little more readable.

Well, I'm no lover of template syntax, but adding type aliases for trivial cases
like this bring more surprises than convenience *for me*. Certainly this is the
simplest type of template that can exist. If it is considered unreadable, then
what kind of templates can there be? 99% of the code is <4>, though. Your call.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLPro...
File src/gpu/gl/GrGLProgram.cpp (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLPro...
src/gpu/gl/GrGLProgram.cpp:297: coeff = GrGLSLExpr<4>::VectorCast(1 -
inColor.a());
On 2013/10/02 15:18:04, bsalomon wrote:
> This line looks a bit odd to me. Does the 1 get casted to a GrGLSLExpr<1>?

It will use the Expr<M> operator-(int, Expr<M>).
This is part of the design where "1" is the tag that means expression with all
ones.

Ints are not convertible to Exprs because const char* are, ie. Expr e = 0 is
ambiguous. Also ints convertible to something else seem to sometimes cause
unforseen problems.

If desired, const char* do not need to be convertible after removing few
instances of passing expressions / variable names as const char*..

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:74: inline const char* GrGLSLTypeString(GrSLType t) {
On 2013/10/02 13:20:26, bsalomon wrote:
> probably want both static and inline here, right?
Well, maybe from standpoint of being consistent with the rest of the header...

I just didn't see why there would need to have multiple versions of the same
function in different translation units?

Changed it to "static inline". (the code is dead anyway)

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:118: SkASSERT(!isValid());
On 2013/10/02 13:20:26, bsalomon wrote:
> this->isValid()
> 
> several more below

Done.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:121: explicit GrGLSLExpr(int v) {
On 2013/10/02 13:20:26, bsalomon wrote:
> 
> Comment that this replicates the int to all components?

Done.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:128: fExpr.appendf(castIntStr(), v);
On 2013/10/02 13:20:26, bsalomon wrote:
> this->castIntStr()

Done.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:154: /** Given an expression of vecM, cast it to vecN. */
On 2013/10/02 13:20:26, bsalomon wrote:
> Document what happens if N > M?
...

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:161: template<int M>
On 2013/10/02 13:20:26, bsalomon wrote:
> Maybe a comment here about how a vecN and vecM are combined?

.. I see what you mean, good point. Changed the interface to not need the
hand-waving..

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:168: /**
On 2013/10/02 15:18:04, bsalomon wrote:
> Maybe we ought to go ahead and have r() g() and b()?
Well, they'd be dead code... 

I didn't add them yet. If you feel they're a must, I can of course do it.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL_...
File src/gpu/gl/GrGLSL_impl.h (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL_...
src/gpu/gl/GrGLSL_impl.h:13: inline const char* GrGLSLExpr<4>::zerosStr() {
On 2013/10/02 13:20:26, bsalomon wrote:
> should zeros and ones be static?
I'm not sure what you mean? At the moment, they are static member functions..

[bsalomon, 2013-10-08 14:45:27]

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/effects/SkArit...
File src/effects/SkArithmeticMode.cpp (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/effects/SkArit...
src/effects/SkArithmeticMode.cpp:371: builder->fsCodeAppendf("\t\tconst vec4 src
= %s;\n", GrGLSLExpr<4>(1).c_str());
On 2013/10/08 12:18:29, kkinnunen wrote:
> On 2013/10/02 15:18:04, bsalomon wrote:
> > Hmm.. This feels a little bit less readable.
> 
> I fixed this and the other similar instance by just not using the func, since
> these didn't need it ever anyway. Would that be ok?

Yes, that seems like the most readable to me.

> 
> > I'd say we should keep the old func or have a globally accessible const
> > GrGLExpr<4> initialized to all 1s:
> > 
> > GrGLExpr<4>::Ones()
> 
> The Ones constructor is not nearly as useful as it initially appears. If you
> want to see the pattern, look at the first patch.
> The problem with it is that you can't do all the equivalent calculation,
namely
> the "1 minus alpha" case. The alpha accessor returns instance of a type that
can
> not be referred to while preserving idiomatic syntax. The "1" is useful when
it
> represents ones of all dimensions, though I'm not sure I'm able to argue/talk
> about this in any type-theoretic way.
> 
> Current "unreadable" usecase is solely "return string of vec4(1)". All the
> current cases can be removed when inputColor is being passed as an expression.
> Most of the future cases that I can think of will need that string in context
of
> big glsl statement blocks, where the author has written the var4 statement
too.
> In these cases the 1,1,1,1 string can and should be written manually. Eg.
> there's no need to use the possible variability that the dimension variable
> would provide.
> 
> Then the "unreadable" concern becomes issue of which of the following is
> idiomatic:
> 
> GrGLSLExpr<4> coeff(1);  // A
> GrGLSLExpr<4> coeff = GrGLSLExpr<4>::Ones; // B
> 
> As far as readibility goes, I think the case A is more readable as there's
less
> to look at. Your call.

That makes sense to me. You're right, we definitely should be passing the input
color to the effects as a GrGLSLExpr.

> 
> If you want the Ones constructor:
> Other working option that I can think of is the pattern where "One"
constructor
> would return a new type GrGLSLExprOnes which then would be convertible to Expr
> of any dimension. I didn't go this route as it seemed to add template bloat.
> 
> > Also, what do you think of:
> > typedef GrGLSLExpr<4> GrGLSLExpr4?
> > I think it might make the code a little more readable.
> 
> Well, I'm no lover of template syntax, but adding type aliases for trivial
cases
> like this bring more surprises than convenience *for me*. Certainly this is
the
> simplest type of template that can exist. If it is considered unreadable, then
> what kind of templates can there be? 99% of the code is <4>, though. Your
call.
> 
> 

I find the <4> slightly distracting visually but I don't have a strong opinion
on this. If you prefer keeping it as is, I don't object.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLPro...
File src/gpu/gl/GrGLProgram.cpp (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLPro...
src/gpu/gl/GrGLProgram.cpp:297: coeff = GrGLSLExpr<4>::VectorCast(1 -
inColor.a());
On 2013/10/08 12:18:29, kkinnunen wrote:
> On 2013/10/02 15:18:04, bsalomon wrote:
> > This line looks a bit odd to me. Does the 1 get casted to a GrGLSLExpr<1>?
> 
> It will use the Expr<M> operator-(int, Expr<M>).
> This is part of the design where "1" is the tag that means expression with all
> ones.
> 

Ah-ha! I missed that operator on first read. This feels a bit too auto-magical
to me. It isn't totally obvious how it works to a causal reader. And going much
further in this direction could lead to code that is pretty hard to read. We
could add float variants and other mixed-type operators and wind up with long
expressions that are mixes of string, exprs, ints, floats that turn into a lot
of conversions or operators. It doesn't seem too onerous to require the caller
to type:

GrGLSLExpr<1>(1) - inColor.a()
instead of
1 - inColor.a()

Maybe another way of stating my concern is that Skia has deliberately steered
clear of a lot of operator overloading in order to make the code more explicit
about what functions are being called to a reader. I think GrGLSLExpr is an
exception because something like

d.div(a.mul(c).add(b)) is so much harder to read than

d / (a*c + b)

but when the types of a, b, c, and d are different I feel like it starts to
become hard to know exactly what's going on. I know that this one operator
doesn't exactly create this situation, but I feel like it is nudging us down
that path.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:74: inline const char* GrGLSLTypeString(GrSLType t) {
On 2013/10/08 12:18:29, kkinnunen wrote:
> On 2013/10/02 13:20:26, bsalomon wrote:
> > probably want both static and inline here, right?
> Well, maybe from standpoint of being consistent with the rest of the header...
> 
> I just didn't see why there would need to have multiple versions of the same
> function in different translation units?
> 
> Changed it to "static inline". (the code is dead anyway)

If it's dead let's remove it.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:168: /**
On 2013/10/08 12:18:29, kkinnunen wrote:
> On 2013/10/02 15:18:04, bsalomon wrote:
> > Maybe we ought to go ahead and have r() g() and b()?
> Well, they'd be dead code... 
> 
> I didn't add them yet. If you feel they're a must, I can of course do it.

No, I just assume if we start really using this a lot in effects and what not
they will be needed. I'm ok if it's just a() but it'd be really weird to say
have r() and a() but not g() and b().

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:141: GrGLSLExpr(const char expr[]) {
explicit? What's the use case for implicit conversion?

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:154: GrGLSLExpr(const SkString& expr) {
ditto.

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:198: static const char* zerosStr();
The reason I thought these were non-static is that our convention is to
capitalize the first letter of static functions.

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:205: static GrGLSLExpr<N> VectorCast(const GrGLSLExpr<M>&
expr);
Document what happens when N>M?

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:208: static GrGLSLExpr<N> mul(const GrGLSLExpr<N>& in0,
const GrGLSLExpr<M>& in1);
comment about what it means to mul, add, sub different lenght vectors?

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:227: friend GrGLSLExpr<4> operator*(const GrGLSLExpr<4>&,
const GrGLSLExpr<1>&);
Should these be limited to 4 for the first input and output? It seems equally
useful for 2 and 3 vecs to operate with scalars.

Also, I think + and - deserve a comment since addition and subtracting a vector
with a scalar is not nearly as common as multiplying a vector and scalar.

[Kimmo Kinnunen, 2013-10-09 13:39:59]

Thanks.
New version. There's a lot of text. If you're in hurry, I'm primarily interested
in the clarifying question about the  usage of "static" keyword for functions in
header files.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLPro...
File src/gpu/gl/GrGLProgram.cpp (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLPro...
src/gpu/gl/GrGLProgram.cpp:297: coeff = GrGLSLExpr<4>::VectorCast(1 -
inColor.a());
On 2013/10/08 14:45:27, bsalomon wrote:
> On 2013/10/08 12:18:29, kkinnunen wrote:
> > On 2013/10/02 15:18:04, bsalomon wrote:
> > > This line looks a bit odd to me. Does the 1 get casted to a GrGLSLExpr<1>?
> > 
> > It will use the Expr<M> operator-(int, Expr<M>).
> > This is part of the design where "1" is the tag that means expression with
all
> > ones.
> > 
> 
> Ah-ha! I missed that operator on first read. This feels a bit too auto-magical
> to me. 
..

> It doesn't seem too onerous to require the caller to type:
> 
> GrGLSLExpr<1>(1) - inColor.a()
> instead of
> 1 - inColor.a()

This was not the rationale. The rationale was the "impossibility" to implement
use-cases that are useful, maybe even the primary rationale for having the Expr,
e.g. to use the algorithms on Expr"ish" objects, in generic way.

Example of the problem:

 template <Expr>
 inv_alpha_adder(const Expr& a, const Expr& b) {
   return a + (ExprWITHDDIMENSIONOFALPHA(1) - b.a());
 }

 SLExpr<4> r = inv_alpha_adder(SLExpr<4>(1), SLExpr<4>(2))
 
Eg. to reduce the dimension, you do as follows -->

 template <Expr, AlphaOner>
 inv_alpha_adder(const Expr& a, const Expr& b, const AlphaOner& one) {
   return a + (one() - b.a());
 }

 class Oner {
 public:
   SLExpr<1> operator() const { return SLExpr<1>(1); }
 }

 Oner oner;
 SLExpr<4> r = inv_alpha_adder(SLExpr<4>(1), SLExpr<4>(2), oner)
 
Though, I'm no template expert. There might be better ways.

Of course, you can also contest the need for the generality ("if you need
inv_alpha_adder with non-SLExpr, re-write it").
 
> I think GrGLSLExpr is an exception because something like
> 
> d.div(a.mul(c).add(b)) is so much harder to read than
> 
> d / (a*c + b)
> 
> but when the types of a, b, c, and d are different I feel like it starts to
> become hard to know exactly what's going on. 

For the actual content, GLSL, the writer naturally has to know what's going on
wrt the implicit type casts. Most likely how that translates to C++ templates
will surprise him, in all more complex cases.

I'm not saying "exprs all the way", per se, but extending the system with none
of the hairy stuff might be unrealistic. This in turn might make future-proofing
slightly frustrating argument: the future that is used as a premise would be
order of magnitude more hairy or the extension would not happen, both
invalidating the future-proofing as an argument.

I removed the operator and used your suggestion. The problem, if there is one,
is not visible in this patch.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:74: inline const char* GrGLSLTypeString(GrSLType t) {
On 2013/10/08 14:45:27, bsalomon wrote:
> On 2013/10/08 12:18:29, kkinnunen wrote:
> > On 2013/10/02 13:20:26, bsalomon wrote:
> > > probably want both static and inline here, right?
> > I just didn't see why there would need to have multiple versions of the same
> > function in different translation units?
> > Changed it to "static inline". (the code is dead anyway)
> If it's dead let's remove it.


Sorry, mistook it for the other dead code. I removed the other dead code,
though.

I added "static inline" vs just "inline".

Just to verify: Using the "static inline" in this .h file will cause creation of
a new function for every .cpp file that uses the function "GrGLSLTypeString". Is
this really intended?

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:141: GrGLSLExpr(const char expr[]) {
On 2013/10/08 14:45:27, bsalomon wrote:
> explicit? What's the use case for implicit conversion?

Mostly due to effects inputting char*s instead of exprs. Added a TODO.

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:198: static const char* zerosStr();
On 2013/10/08 14:45:27, bsalomon wrote:
> The reason I thought these were non-static is that our convention is to
> capitalize the first letter of static functions.

Done.

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:205: static GrGLSLExpr<N> VectorCast(const GrGLSLExpr<M>&
expr);
On 2013/10/08 14:45:27, bsalomon wrote:
> Document what happens when N>M?

I'll add the docstrings back. See below, though..

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:208: static GrGLSLExpr<N> mul(const GrGLSLExpr<N>& in0,
const GrGLSLExpr<M>& in1);
On 2013/10/08 14:45:27, bsalomon wrote:
> comment about what it means to mul, add, sub different lenght vectors?

Well, I switched from member operators to private member functions in with the
expectation that private member functions wouldn't need comments.. Users would
just look at the interface definition, and no questions would occur. I'll add
the comments back.

https://chromiumcodereview.appspot.com/25048002/diff/50001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:227: friend GrGLSLExpr<4> operator*(const GrGLSLExpr<4>&,
const GrGLSLExpr<1>&);
On 2013/10/08 14:45:27, bsalomon wrote:
> Should these be limited to 4 for the first input and output? It seems equally
> useful for 2 and 3 vecs to operate with scalars.

Intention was that once they're used, they're added.
If you meant to template it: it's not possible AFAIK, as the N=1, M=1 would
create ambiguous operators.

> Also, I think + and - deserve a comment since addition and subtracting a
vector
> with a scalar is not nearly as common as multiplying a vector and scalar.

The comments were below at the operator definition. I moved them near the friend
declarations?

[bsalomon, 2013-10-09 19:53:31]

lgtm. I noticed two "this->"s that should be removed (noted inline below). You
should be able to make that change, upload a new patch, and hit the commit box
if you like. Our code review system doesn't remember what patchset got the
"lgt"m comment!

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLPro...
File src/gpu/gl/GrGLProgram.cpp (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLPro...
src/gpu/gl/GrGLProgram.cpp:297: coeff = GrGLSLExpr<4>::VectorCast(1 -
inColor.a());
I'm not 100% sure that I follow the example but I think we should defer until
the need arises for more complexity.

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

https://chromiumcodereview.appspot.com/25048002/diff/10001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:74: inline const char* GrGLSLTypeString(GrSLType t) {
On 2013/10/09 13:39:59, kkinnunen wrote:
> On 2013/10/08 14:45:27, bsalomon wrote:
> > On 2013/10/08 12:18:29, kkinnunen wrote:
> > > On 2013/10/02 13:20:26, bsalomon wrote:
> > > > probably want both static and inline here, right?
> > > I just didn't see why there would need to have multiple versions of the
same
> > > function in different translation units?
> > > Changed it to "static inline". (the code is dead anyway)
> > If it's dead let's remove it.
> 
> 
> Sorry, mistook it for the other dead code. I removed the other dead code,
> though.
> 
> I added "static inline" vs just "inline".
> 
> Just to verify: Using the "static inline" in this .h file will cause creation
of
> a new function for every .cpp file that uses the function "GrGLSLTypeString".
Is
> this really intended?
> 

Hmm..  Perhaps not. Though, if the function really is always inlined then it
doesn't matter. Maybe we should consider making some of our "static inline"
functions just "inline" but there are many similar examples in Skia and if we
make the change it should be a broader migration.

https://chromiumcodereview.appspot.com/25048002/diff/55001/src/gpu/gl/GrGLSL.h
File src/gpu/gl/GrGLSL.h (right):

https://chromiumcodereview.appspot.com/25048002/diff/55001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:143: return this->ZerosStr();
Ok, so maybe I asked you to do this-> here before, but I didn't realize the func
was static at that time. :( We don't use the this-> convention for static func
calls.

https://chromiumcodereview.appspot.com/25048002/diff/55001/src/gpu/gl/GrGLSL....
src/gpu/gl/GrGLSL.h:145: return this->OnesStr();
and here

[commit-bot: I haz the power, 2013-10-10 06:20:48]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/25048002/78001

[commit-bot: I haz the power, 2013-10-10 06:30:22]

Change committed as 11690