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

src/effects/SkArithmeticMode.cpp

 /*
  * Copyright 2013 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkArithmeticMode.h"
 #include "SkColorPriv.h"
 #include "SkFlattenableBuffers.h"
@@ skipping 350 matching lines @@
         dstColor = builder->dstColor();
     }
 
     SkASSERT(NULL != dstColor);
     fKUni = builder->addUniform(GrGLShaderBuilder::kFragment_Visibility,
                                 kVec4f_GrSLType, "k");
     const char* kUni = builder->getUniformCStr(fKUni);
 
     // We don't try to optimize for this case at all
     if (NULL == inputColor) {
-        builder->fsCodeAppendf("\t\tconst vec4 src = %s;\n", GrGLSLOnesVecf(4));
+        builder->fsCodeAppendf("\t\tconst vec4 src = %s;\n", GrGLSLExpr<4>(1).c_str());

[bsalomon, 2013/10/02 15:18:04]

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.

[Kimmo Kinnunen, 2013/10/08 12:18:29]

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?
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.
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.

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

Yes, that seems like the most readable to me.
That makes sense to me. You're right, we definitely should be passing the input
color to the effects as a GrGLSLExpr.
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.

     } else {
         builder->fsCodeAppendf("\t\tvec4 src = %s;\n", inputColor);
         if (gUseUnpremul) {
             builder->fsCodeAppendf("\t\tsrc.rgb = clamp(src.rgb / src.a, 0.0, 1.0);\n");
         }
     }
 
     builder->fsCodeAppendf("\t\tvec4 dst = %s;\n", dstColor);
     if (gUseUnpremul) {
         builder->fsCodeAppendf("\t\tdst.rgb = clamp(dst.rgb / dst.a, 0.0, 1.0);\n");
@@ skipping 61 matching lines @@
                                              background);
     }
     return true;
 }
 
 #endif
 
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_START(SkArithmeticMode)
     SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkArithmeticMode_scalar)
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_END