Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12844006:
reformatting shader.cc and shader.h (Closed)
|
Created:
7 years, 9 months ago by whunt Modified:
7 years, 9 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionreformatting shader.cc and shader.h
Patch Set 1 #Patch Set 2 : reverting unintentional, unrelated changes #
Total comments: 5
Patch Set 3 : updating constructor initializer list style #
Total comments: 4
Patch Set 4 : fixing 80-column errors introduced during macro-replace #
Messages
Total messages: 18 (0 generated)
Just so you know: if you don't post a comment, it doesn't send any email, and folks using an email-based workflow will likely not see your patch.
https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc#newcode64 cc/output/shader.cc:64: base_uniform_index); note: this is valid style, but you can also do more concisely: GetProgramUniformLocations( context, program, shader_uniforms, arraysize(shader_uniforms), arraysize(locations), locations, using_bind_uniform, base_uniform_index); Either way works. https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc#newcode85 cc/output/shader.cc:85: , tex_scale_location_(-1) { } nit: chrome style for initializers: VertexShaderPosTexYUVStretch::VertexShaderPosTexYUVStretch() : matrix_location_(-1), tex_scale_location_(-1) {} Note: ':' is at +4, the ',' is at the end of the line instead of the beginning, no space necessary within empty brackets. Several other instances below. https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc#newcod... cc/output/shader.cc:767: maskTexCoordScale.y); I think it's a bit more readable / less confusing to write it as: vec2 maskTexCoord = vec2( maskTexCoordOffset.x + v_texCoord.x * maskTexCoordScale.x, maskTexCoordOffset.y + v_texCoord.y * maskTexCoordScale.y);
https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc#newcode85 cc/output/shader.cc:85: , tex_scale_location_(-1) { } http://go/clang-format will fix this automatically for you.
https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc#newcode64 cc/output/shader.cc:64: base_uniform_index); On 2013/03/20 07:01:41, piman wrote: > note: this is valid style, but you can also do more concisely: > GetProgramUniformLocations( > context, program, shader_uniforms, arraysize(shader_uniforms), > arraysize(locations), locations, using_bind_uniform, base_uniform_index); > Either way works. We've been following the rule of "put it all on one line, or only put one thing per line"
On Wed, Mar 20, 2013 at 9:51 AM, <danakj@chromium.org> wrote: > > https://codereview.chromium.**org/12844006/diff/2001/cc/**output/shader.cc<ht... > File cc/output/shader.cc (right): > > https://codereview.chromium.**org/12844006/diff/2001/cc/** > output/shader.cc#newcode64<https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc#newcode64> > cc/output/shader.cc:64: base_uniform_index); > > On 2013/03/20 07:01:41, piman wrote: > >> note: this is valid style, but you can also do more concisely: >> GetProgramUniformLocations( >> context, program, shader_uniforms, arraysize(shader_uniforms), >> arraysize(locations), locations, using_bind_uniform, >> > base_uniform_index); > >> Either way works. >> > > We've been following the rule of "put it all on one line, or only put > one thing per line" > The style guide allows more styles than this: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls If there is some cc-specific style that differs from the general style guide, they should be well communicated with the rationale: - they should be described in the top-level README - they should be communicated on the mailing list Otherwise: - some reviewers/OWNERs won't know these rules and let through CLs that don't follow this style - everybody wastes time by doing extra rounds of review. Do you think deviating from the style guide of the rest of the code base has a strong enough rationale? > https://codereview.chromium.**org/12844006/<https://codereview.chromium.org/1... >
On Wed, Mar 20, 2013 at 10:12 AM, Antoine Labour <piman@chromium.org> wrote: > > > > On Wed, Mar 20, 2013 at 9:51 AM, <danakj@chromium.org> wrote: > >> >> https://codereview.chromium.**org/12844006/diff/2001/cc/** >> output/shader.cc<https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc> >> File cc/output/shader.cc (right): >> >> https://codereview.chromium.**org/12844006/diff/2001/cc/** >> output/shader.cc#newcode64<https://codereview.chromium.org/12844006/diff/2001/cc/output/shader.cc#newcode64> >> cc/output/shader.cc:64: base_uniform_index); >> >> On 2013/03/20 07:01:41, piman wrote: >> >>> note: this is valid style, but you can also do more concisely: >>> GetProgramUniformLocations( >>> context, program, shader_uniforms, arraysize(shader_uniforms), >>> arraysize(locations), locations, using_bind_uniform, >>> >> base_uniform_index); >> >>> Either way works. >>> >> >> We've been following the rule of "put it all on one line, or only put >> one thing per line" >> > > The style guide allows more styles than this: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls > > > If there is some cc-specific style that differs from the general style > guide, they should be well communicated with the rationale: > - they should be described in the top-level README > - they should be communicated on the mailing list > > Otherwise: > - some reviewers/OWNERs won't know these rules and let through CLs that > don't follow this style > - everybody wastes time by doing extra rounds of review. > > Do you think deviating from the style guide of the rest of the code base > has a strong enough rationale? > Chromium style guide says this: - For function declarations and definitions, put each argument on a separate line unless the whole declaration fits on one line. void OK1(TypeA first_argument, TypeB second_argument, TypeC third_argument); void OK2(TypeA first_argument, TypeB second_argument, TypeC third_argument); http://dev.chromium.org/developers/coding-style That overrides the Google style guide. - James > > >> https://codereview.chromium.**org/12844006/<https://codereview.chromium.org/1... >> > >
On 2013/03/20 17:25:53, jamesr1 wrote: > Chromium style guide says this: > > > - For function declarations and definitions, put each argument on a > separate line unless the whole declaration fits on one line. > > void OK1(TypeA first_argument, TypeB second_argument, TypeC third_argument); > void OK2(TypeA first_argument, > TypeB second_argument, > > TypeC third_argument); > > http://dev.chromium.org/developers/coding-style > > That overrides the Google style guide. That's for declarations and definitions. This is about calling code. I agree with danakj that if it doesn't fit on one line, I often prefer to have one per line, mostly since a function that has so many parameters will often accrete more and it makes it easier to add. Also, this seems to be what clang-format does automatically. It's not some sort of hard style rule in cc, just an optional recommendation for readability within the existing style guidelines.
https://codereview.chromium.org/12844006/diff/9001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12844006/diff/9001/cc/output/gl_renderer.cc#n... cc/output/gl_renderer.cc:360: SetShaderOpacity(quad->opacity(), program->fragment_shader().alpha_location()); 80 columns. https://codereview.chromium.org/12844006/diff/9001/cc/output/gl_renderer.cc#n... cc/output/gl_renderer.cc:1241: SetShaderOpacity(quad->opacity(), program->fragment_shader().alpha_location()); 80 columns. https://codereview.chromium.org/12844006/diff/9001/cc/output/gl_renderer.cc#n... cc/output/gl_renderer.cc:1273: SetShaderOpacity(quad->opacity(), program->fragment_shader().alpha_location()); 80 columns. https://codereview.chromium.org/12844006/diff/9001/cc/output/gl_renderer.cc#n... cc/output/gl_renderer.cc:1300: vertex_opacity_location = program->vertex_shader().vertex_opacity_location(); 80 columns.
lgtm, committing Can you *please* add a comment to the code review when you have updated a patch in response to comments and want somebody to look at it?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/whunt@chromium.org/12844006/15001
On 2013/03/21 17:46:53, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/whunt%40chromium.org/12844006/15001 It doesn't send mail when new patch sets are uploaded?? Given your comment I'll assume not and get in the habit of sending a message. I'm just a little surprised it doesn't alert you via email, that seems like an obvious feature request...
On Thu, Mar 21, 2013 at 10:54 AM, <whunt@chromium.org> wrote: > Reviewers: piman, danakj, jamesr, enne, jamesr1, > > Message: > > On 2013/03/21 17:46:53, I haz the power (commit-bot) wrote: > >> CQ is trying da patch. Follow status at >> https://chromium-status.**appspot.com/cq/whunt%** >> 40chromium.org/12844006/15001<https://chromium-status.appspot.com/cq/whunt%40chromium.org/12844006/15001> >> > > It doesn't send mail when new patch sets are uploaded?? Given your > comment I'll > assume not and get in the habit of sending a message. I'm just a little > surprised it doesn't alert you via email, that seems like an obvious > feature > request... > It doesn't - never has. - James > > Description: > reformatting shader.cc and shader.h > > > Please review this at https://codereview.chromium.**org/12844006/<https://codereview.chromium.org/1... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M cc/output/gl_renderer.cc > M cc/output/program_binding.h > M cc/output/shader.h > M cc/output/shader.cc > > >
On 2013/03/21 17:57:43, jamesr1 wrote: > On Thu, Mar 21, 2013 at 10:54 AM, <mailto:whunt@chromium.org> wrote: > > > Reviewers: piman, danakj, jamesr, enne, jamesr1, > > > > Message: > > > > On 2013/03/21 17:46:53, I haz the power (commit-bot) wrote: > > > >> CQ is trying da patch. Follow status at > >> https://chromium-status.**appspot.com/cq/whunt%25** > >> > 40chromium.org/12844006/15001<https://chromium-status.appspot.com/cq/whunt%40chromium.org/12844006/15001> > >> > > > > It doesn't send mail when new patch sets are uploaded?? Given your > > comment I'll > > assume not and get in the habit of sending a message. I'm just a little > > surprised it doesn't alert you via email, that seems like an obvious > > feature > > request... > > > > It doesn't - never has. > > - James > > > > > > Description: > > reformatting shader.cc and shader.h > > > > > > Please review this at > https://codereview.chromium.**org/12844006/%3Chttps://codereview.chromium.org...> > > > > SVN Base: > svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > > > Affected files: > > M cc/output/gl_renderer.cc > > M cc/output/program_binding.h > > M cc/output/shader.h > > M cc/output/shader.cc > > > > > > boo. I guess I never noticed... I haven't been a reviewer frequently yet.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/whunt@chromium.org/12844006/15001
When I tried to apply this patch locally, I got conflicts with https://chromiumcodereview.appspot.com/13008010 which just landed. Does this patch need to be rebased?
If this patch doesn't make the CQ, I have a rebased version here: https://codereview.chromium.org/12764030. Feel free to upload it to this issue.
Retried try job too often on win7_aura for step(s) ash_unittests, aura_unittests, browser_tests, compositor_unittests, content_browsertests, content_unittests, interactive_ui_tests, unit_tests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Closing because brianderson's rebased patch landed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
