Pretty print of shaders

Pretty print of shaders

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/bab82ed05b304504e3b5d317486e152dce107313

[joshua.litt, 2014-08-01 00:33:37]

I'm not super proud of this.  I debated writing a more complete parser with
tokenizer, but to be honest in the long term I really want to overhaul our
shader system to use C++  wrapper classes for glsl types + overloaded operators.
 I don't think its worth spending too much time on this.

Also, my line estimate was off by a factor of 4 because I forgot about comments
/ preprocessor directives

[bsalomon, 2014-08-01 13:37:06]

We have a unit test that generates programs with random combinations of effects,
GLProgramsTest. It'd be good to run this against those generated programs to
make sure it doesn't crash and that the pretty printed shaders compile (in
addition to the ugly printed). It might also be useful for a one time manual
testing to make sure they look right.

https://codereview.chromium.org/437593004/diff/40002/gyp/gpu.gypi
File gyp/gpu.gypi (right):

https://codereview.chromium.org/437593004/diff/40002/gyp/gpu.gypi#newcode205
gyp/gpu.gypi:205: '<(skia_src_path)/gpu/gl/GrGLPrettyPrintSL.h',
I think it's slightly more consistent with other names in Gr code to call this
GrGLSLPrettyPrint.

https://codereview.chromium.org/437593004/diff/40002/src/gpu/gl/GrGLPrettyPri...
File src/gpu/gl/GrGLPrettyPrintSL.cpp (right):

https://codereview.chromium.org/437593004/diff/40002/src/gpu/gl/GrGLPrettyPri...
src/gpu/gl/GrGLPrettyPrintSL.cpp:1: #include "GrGLPrettyPrintSL.h"
Copyright

https://codereview.chromium.org/437593004/diff/40002/src/gpu/gl/GrGLPrettyPri...
File src/gpu/gl/GrGLPrettyPrintSL.h (right):

https://codereview.chromium.org/437593004/diff/40002/src/gpu/gl/GrGLPrettyPri...
src/gpu/gl/GrGLPrettyPrintSL.h:1: #ifndef GRGLPRETTYPRINTSL_H_
copyright notice.

Also, our convention is GrGLPrettyPrintDL_DEFINED for these

https://codereview.chromium.org/437593004/diff/40002/src/gpu/gl/GrGLPrettyPri...
src/gpu/gl/GrGLPrettyPrintSL.h:7: SkString pretty_print_sl(const SkString&
input, bool countlines);
PrettyPrint()? I think we usually use initial-cap-camel-case for namespace
functions (matching static class methods).

[joshua.litt, 2014-08-01 21:46:47]

cleaned up, feeback incorporated

[Stephen White, 2014-08-04 15:10:29]

I didn't look into this in depth (seems fine); just added a high-level comment.

https://codereview.chromium.org/437593004/diff/150001/src/gpu/gl/GrGLSLPretty...
File src/gpu/gl/GrGLSLPrettyPrint.cpp (right):

https://codereview.chromium.org/437593004/diff/150001/src/gpu/gl/GrGLSLPretty...
src/gpu/gl/GrGLSLPrettyPrint.cpp:53: if (hasToken("#") || hasToken("//")) {
I guess ad hoc parsing is ok for this, since we know most of the time that the
input is conforming. If not, we might want to consider a real lexer.

OTOH, maybe we should just enforce the formatting in the shader generator
itself, and stop putting whitespace in the call sites at all. Just a thought.

[bsalomon, 2014-08-04 15:20:02]

https://codereview.chromium.org/437593004/diff/150001/src/gpu/gl/GrGLSLPretty...
File src/gpu/gl/GrGLSLPrettyPrint.cpp (right):

https://codereview.chromium.org/437593004/diff/150001/src/gpu/gl/GrGLSLPretty...
src/gpu/gl/GrGLSLPrettyPrint.cpp:11: class GrGLSLPrettyPrint {
For classes that are private to a particular translation unit we don't usually
use the Gr prefix. That's also true for classes that are nested within another
Gr-prefixed class or Gr-prefixed namespace.

https://codereview.chromium.org/437593004/diff/150001/src/gpu/gl/GrGLSLPretty...
src/gpu/gl/GrGLSLPrettyPrint.cpp:53: if (hasToken("#") || hasToken("//")) {
On 2014/08/04 15:10:29, Stephen White wrote:
> I guess ad hoc parsing is ok for this, since we know most of the time that the
> input is conforming. If not, we might want to consider a real lexer.
> 
> OTOH, maybe we should just enforce the formatting in the shader generator
> itself, and stop putting whitespace in the call sites at all. Just a thought.

Part of the advantage of having a separate function is that we don't worry about
it at all in a normal release run. The effects can optionally add whatever
whitespace they want, or not, and we just pass the ugly shader string down to
GL. We only pretty print in debug builds with shader dumping enabled or when a
shader failed to compile/link.

Because the Debug builds of our regression test tools will actually send the
pretty-printed string to GL we should catch any differences in language or
logical correctness introduced by the pretty printer.

https://codereview.chromium.org/437593004/diff/150001/src/gpu/gl/GrGLSLPretty...
src/gpu/gl/GrGLSLPrettyPrint.cpp:55: } else if (hasToken("/*")) {
minor style nit: this->hasToken() (and similar in other places). We only do that
for methods, not data members.

[joshua.litt, 2014-08-07 16:09:42]

Feedback incorporated

[bsalomon, 2014-08-07 20:37:29]

lgtm

[joshua.litt, 2014-08-08 16:19:28]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-08-08 16:20:29]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/joshualitt@chromium.org/437593004/170002

[commit-bot: I haz the power, 2014-08-08 16:27:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)

[commit-bot: I haz the power, 2014-08-08 16:30:29]

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

[commit-bot: I haz the power, 2014-08-08 16:30:30]

Try jobs failed on following builders:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)

[joshua.litt, 2014-08-08 16:34:05]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-08-08 16:34:29]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/joshualitt@chromium.org/437593004/200001

[commit-bot: I haz the power, 2014-08-08 16:41:47]

Change committed as bab82ed05b304504e3b5d317486e152dce107313