Issue 698041: Added GLSL backend for ESSL translator. (Closed)
|
Created:
14 years, 2 months ago by Alok Priyadarshi Modified:
14 years, 1 month ago CC:
angleproject-review_googlegroups.com Base URL:
http://angleproject.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdded GLSL backend for ESSL translator.
Patch Set 1 #
Total comments: 27
Patch Set 2 : '' #
Total comments: 6
Patch Set 3 : '' #
MessagesTotal messages: 18
Sign in to reply to this message.
Mostly comments about style issues. http://codereview.appspot.com/698041/diff/1/2 File samples/samples.sln (right): http://codereview.appspot.com/698041/diff/1/2#newcode72 samples/samples.sln:72: ProjectSection(ProjectDependencies) = postProject Was this section hand-edited? I wouldn't expect to see tabs here. http://codereview.appspot.com/698041/diff/1/3 File samples/translator/translator.cpp (right): http://codereview.appspot.com/698041/diff/1/3#newcode82 samples/translator/translator.cpp:82: case 'i': debugOptions |= EDebugOpIntermediate; break; there seem to be some issues with the alignment of the case statements here. http://codereview.appspot.com/698041/diff/1/3#newcode101 samples/translator/translator.cpp:101: if (! CompileFile(argv[0], compilers[numCompilers-1], debugOptions, &resources)) remove space after ! http://codereview.appspot.com/698041/diff/1/3#newcode180 samples/translator/translator.cpp:180: printf("Usage: standalone [-i -a -c -o -m] file1 file2 ...\n" document the flags here? http://codereview.appspot.com/698041/diff/1/3#newcode190 samples/translator/translator.cpp:190: FILE *in = fopen(fileName, "r"); remove extra spaces before = http://codereview.appspot.com/698041/diff/1/3#newcode193 samples/translator/translator.cpp:193: char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1); add space after ** http://codereview.appspot.com/698041/diff/1/3#newcode206 samples/translator/translator.cpp:206: if (!(fdata = (char *)malloc(count+2))) { remove space between char and * http://codereview.appspot.com/698041/diff/1/3#newcode216 samples/translator/translator.cpp:216: if(count==0){ I notice in general that the spacing you use in expressions is a bit inconsistent. Sometimes you use spaces around = sometimes not, etc. Ideally you should stick with one style. http://codereview.appspot.com/698041/diff/1/3#newcode243 samples/translator/translator.cpp:243: void FreeFileData(char **data) char** data http://codereview.appspot.com/698041/diff/1/3#newcode255 samples/translator/translator.cpp:255: int ShOutputMultipleStrings(char **argv) char** argv It's a style issue but it seems that most guides say the * sticks with the type. http://codereview.appspot.com/698041/diff/1/6 File src/ANGLE.sln (right): http://codereview.appspot.com/698041/diff/1/6#newcode22 src/ANGLE.sln:22: ProjectSection(ProjectDependencies) = postProject tabs, again. http://codereview.appspot.com/698041/diff/1/10 File src/compiler/intermOut.cpp (right): http://codereview.appspot.com/698041/diff/1/10#newcode100 src/compiler/intermOut.cpp:100: case EOpInitialize: out.debug << "initialize first child with second child"; break; watch indentation http://codereview.appspot.com/698041/diff/1/18 File src/compiler/translator_frontend.vcproj (right): http://codereview.appspot.com/698041/diff/1/18#newcode5 src/compiler/translator_frontend.vcproj:5: Name="translator_frontend" tab
Sign in to reply to this message.
what do you guys think about using translator in project names? will it be confusing since the code is under src/compiler? should I stick with compiler in the project name? http://codereview.appspot.com/698041/diff/1/2 File samples/samples.sln (right): http://codereview.appspot.com/698041/diff/1/2#newcode72 samples/samples.sln:72: ProjectSection(ProjectDependencies) = postProject This was auto-generated. Should I hand-edit to remove tags? http://codereview.appspot.com/698041/diff/1/3 File samples/translator/translator.cpp (right): http://codereview.appspot.com/698041/diff/1/3#newcode82 samples/translator/translator.cpp:82: case 'i': debugOptions |= EDebugOpIntermediate; break; On 2010/03/23 22:34:28, vangelis wrote: > there seem to be some issues with the alignment of the case statements here. Done. http://codereview.appspot.com/698041/diff/1/3#newcode101 samples/translator/translator.cpp:101: if (! CompileFile(argv[0], compilers[numCompilers-1], debugOptions, &resources)) On 2010/03/23 22:34:28, vangelis wrote: > remove space after ! Done. http://codereview.appspot.com/698041/diff/1/3#newcode180 samples/translator/translator.cpp:180: printf("Usage: standalone [-i -a -c -o -m] file1 file2 ...\n" On 2010/03/23 22:34:28, vangelis wrote: > document the flags here? Done. http://codereview.appspot.com/698041/diff/1/3#newcode190 samples/translator/translator.cpp:190: FILE *in = fopen(fileName, "r"); On 2010/03/23 22:34:28, vangelis wrote: > remove extra spaces before = Done. http://codereview.appspot.com/698041/diff/1/3#newcode193 samples/translator/translator.cpp:193: char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1); On 2010/03/23 22:34:28, vangelis wrote: > add space after ** Done. http://codereview.appspot.com/698041/diff/1/3#newcode206 samples/translator/translator.cpp:206: if (!(fdata = (char *)malloc(count+2))) { On 2010/03/23 22:34:28, vangelis wrote: > remove space between char and * Done. http://codereview.appspot.com/698041/diff/1/3#newcode216 samples/translator/translator.cpp:216: if(count==0){ This file came with 3dlabs compiler which has inconsistent style. I have tried to fix all I came across. http://codereview.appspot.com/698041/diff/1/3#newcode243 samples/translator/translator.cpp:243: void FreeFileData(char **data) On 2010/03/23 22:34:28, vangelis wrote: > char** data Done. http://codereview.appspot.com/698041/diff/1/3#newcode255 samples/translator/translator.cpp:255: int ShOutputMultipleStrings(char **argv) On 2010/03/23 22:34:28, vangelis wrote: > char** argv > > It's a style issue but it seems that most guides say the * sticks with the type. Done. http://codereview.appspot.com/698041/diff/1/6 File src/ANGLE.sln (right): http://codereview.appspot.com/698041/diff/1/6#newcode22 src/ANGLE.sln:22: ProjectSection(ProjectDependencies) = postProject this file is auto-generated. http://codereview.appspot.com/698041/diff/1/10 File src/compiler/intermOut.cpp (right): http://codereview.appspot.com/698041/diff/1/10#newcode100 src/compiler/intermOut.cpp:100: case EOpInitialize: out.debug << "initialize first child with second child"; break; removed tabs from the whole file and fixed indentation everywhere. http://codereview.appspot.com/698041/diff/1/18 File src/compiler/translator_frontend.vcproj (right): http://codereview.appspot.com/698041/diff/1/18#newcode5 src/compiler/translator_frontend.vcproj:5: Name="translator_frontend" should I hand-edit these files? the tabs will come back as soon as they are touched again.
Sign in to reply to this message.
+kbr
Sign in to reply to this message.
I think "translator" is a good name for what we're trying to do. http://codereview.appspot.com/698041/diff/1/2 File samples/samples.sln (right): http://codereview.appspot.com/698041/diff/1/2#newcode72 samples/samples.sln:72: ProjectSection(ProjectDependencies) = postProject On 2010/03/23 23:24:51, alokp wrote: > This was auto-generated. Should I hand-edit to remove tags? No, no need to. I was just curious how come some sections use spaces and others use tabs.
Sign in to reply to this message.
LGTM. Couple of high-level comments. 1) What is the plan when we are running on an OpenGL ES 2.0 implementation? Will we just feed the shader to the implementation or still at least validate it? Some GLSL ES implementations might be permissive, so it may be worth validating, for example, that floating point precision is specified in the fragment shader. 2) How complete is the translation and validation phase currently? Is some of this code in other files? For example, I don't see any stripping of precision qualifiers in the GLSL translation pass. 3) Run gcl lint and look through the lint errors. While I guess we aren't making this code conform to the Google style, there are still tabs in some of the non-Visual-Studio files, for example. 4) Personally I would prefer the naming convention glsl_es_to_... rather than essl_... but that's a matter of taste. http://codereview.appspot.com/698041/diff/9001/10002 File samples/translator/translator.cpp (right): http://codereview.appspot.com/698041/diff/9001/10002#newcode2 samples/translator/translator.cpp:2: // Copyright (c) 2002-2010 The GATE Project Authors. All rights reserved. GATE -> ANGLE? http://codereview.appspot.com/698041/diff/9001/10020 File src/compiler/OutputGLSL.cpp (right): http://codereview.appspot.com/698041/diff/9001/10020#newcode2 src/compiler/OutputGLSL.cpp:2: // Copyright (c) 2002-2010 The ANGLE Project Authors. All rights reserved. Presumably just 2010? http://codereview.appspot.com/698041/diff/9001/10020#newcode80 src/compiler/OutputGLSL.cpp:80: //symbol->dump(parseContext.infoSink); Remove
Sign in to reply to this message.
(Resending just to get archived in angleproject-review) On 2010/03/24 01:25:20, kbr1 wrote: > LGTM. Couple of high-level comments. > > 1) What is the plan when we are running on an OpenGL ES 2.0 implementation? Will > we just feed the shader to the implementation or still at least validate it? > Some GLSL ES implementations might be permissive, so it may be worth validating, > for example, that floating point precision is specified in the fragment shader. > > 2) How complete is the translation and validation phase currently? Is some of > this code in other files? For example, I don't see any stripping of precision > qualifiers in the GLSL translation pass. > > 3) Run gcl lint and look through the lint errors. While I guess we aren't making > this code conform to the Google style, there are still tabs in some of the > non-Visual-Studio files, for example. > > 4) Personally I would prefer the naming convention glsl_es_to_... rather than > essl_... but that's a matter of taste. > > http://codereview.appspot.com/698041/diff/9001/10002 > File samples/translator/translator.cpp (right): > > http://codereview.appspot.com/698041/diff/9001/10002#newcode2 > samples/translator/translator.cpp:2: // Copyright (c) 2002-2010 The GATE Project > Authors. All rights reserved. > GATE -> ANGLE? > > http://codereview.appspot.com/698041/diff/9001/10020 > File src/compiler/OutputGLSL.cpp (right): > > http://codereview.appspot.com/698041/diff/9001/10020#newcode2 > src/compiler/OutputGLSL.cpp:2: // Copyright (c) 2002-2010 The ANGLE Project > Authors. All rights reserved. > Presumably just 2010? > > http://codereview.appspot.com/698041/diff/9001/10020#newcode80 > src/compiler/OutputGLSL.cpp:80: //symbol->dump(parseContext.infoSink); > Remove
Sign in to reply to this message.
Hi Vangelis, Regarding sticking the * with the type or variable name: If I may put my two cents in, I believe there are strong technical reasons to stick it to the variable name. Here are five real-world situations where this becomes more than a style decision: 1) A declaration like "int* p, q;" does not declare two pointers, it declares a pointer and an integer. And it gets really awkward to have to write "int* p, *q;" when you do want two pointers. By writing "int *p, q;" and "int *p, *q;" both cases are easy to interpret correctly and consistent. 2) Writing "(int*) p;" results in a compilation error. You would think that if "int*" represented a valid type in a declaration you could enclose it with parenthesis. On the other hand "int (*p);" does compile correctly. 3) The asterisk can be regarded as a decoration of the name, and without the decoration you get the pointer. So when writing something like "const int **p;", "**p" is the full name of a constant integer, and indeed it can be used as one. "*p" is the name of a pointer to a constant integer, and "p" is the name of a pointer to a pointer to a constant integer. If instead one would write "const int** p;", something like "*p" has little connection to the declaration (it's hard to tell whether the address is constant or whether the integer is constant). 4) In C/C++ types are always read from the variable name outward. A declaration like "int *const &r;" is a reference to a constant pointer to an integer. Reading it left-to-right instead and trying to interpret it correctly is quite hard if not impossible but you'd be encouraged to do that if it was written as "int* const& r;". And things can get much, much more complicated than this. 5) Something like "int (*p)[2];" declares a pointer to an array of integers, while "int* p[2];" declares an array of pointers. That's something entirely different. Of course you could write "int(* p)[2];" but that quickly becomes nonsensical ascii soup. Likewise the notation for function pointers and even more so method pointers demand placing the asterisk next to the variable name if you want to avoid spaces in the weirdest places. Kind regards, Nicolas -----Original Message----- From: angleproject-review@googlegroups.com [mailto:angleproject-review@googlegroups.com] On Behalf Of vangelis@google.com Sent: Tuesday, March 23, 2010 23:34 To: alokp@chromium.org; daniel@transgaming.com Cc: angleproject-review@googlegroups.com; reply@codereview.appspotmail.com Subject: Re: Added GLSL backend for ESSL translator. (issue698041) Mostly comments about style issues. http://codereview.appspot.com/698041/diff/1/2 File samples/samples.sln (right): http://codereview.appspot.com/698041/diff/1/2#newcode72 samples/samples.sln:72: ProjectSection(ProjectDependencies) = postProject Was this section hand-edited? I wouldn't expect to see tabs here. http://codereview.appspot.com/698041/diff/1/3 File samples/translator/translator.cpp (right): http://codereview.appspot.com/698041/diff/1/3#newcode82 samples/translator/translator.cpp:82: case 'i': debugOptions |= EDebugOpIntermediate; break; there seem to be some issues with the alignment of the case statements here. http://codereview.appspot.com/698041/diff/1/3#newcode101 samples/translator/translator.cpp:101: if (! CompileFile(argv[0], compilers[numCompilers-1], debugOptions, &resources)) remove space after ! http://codereview.appspot.com/698041/diff/1/3#newcode180 samples/translator/translator.cpp:180: printf("Usage: standalone [-i -a -c -o -m] file1 file2 ...\n" document the flags here? http://codereview.appspot.com/698041/diff/1/3#newcode190 samples/translator/translator.cpp:190: FILE *in = fopen(fileName, "r"); remove extra spaces before = http://codereview.appspot.com/698041/diff/1/3#newcode193 samples/translator/translator.cpp:193: char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1); add space after ** http://codereview.appspot.com/698041/diff/1/3#newcode206 samples/translator/translator.cpp:206: if (!(fdata = (char *)malloc(count+2))) { remove space between char and * http://codereview.appspot.com/698041/diff/1/3#newcode216 samples/translator/translator.cpp:216: if(count==0){ I notice in general that the spacing you use in expressions is a bit inconsistent. Sometimes you use spaces around = sometimes not, etc. Ideally you should stick with one style. http://codereview.appspot.com/698041/diff/1/3#newcode243 samples/translator/translator.cpp:243: void FreeFileData(char **data) char** data http://codereview.appspot.com/698041/diff/1/3#newcode255 samples/translator/translator.cpp:255: int ShOutputMultipleStrings(char **argv) char** argv It's a style issue but it seems that most guides say the * sticks with the type. http://codereview.appspot.com/698041/diff/1/6 File src/ANGLE.sln (right): http://codereview.appspot.com/698041/diff/1/6#newcode22 src/ANGLE.sln:22: ProjectSection(ProjectDependencies) = postProject tabs, again. http://codereview.appspot.com/698041/diff/1/10 File src/compiler/intermOut.cpp (right): http://codereview.appspot.com/698041/diff/1/10#newcode100 src/compiler/intermOut.cpp:100: case EOpInitialize: out.debug << "initialize first child with second child"; break; watch indentation http://codereview.appspot.com/698041/diff/1/18 File src/compiler/translator_frontend.vcproj (right): http://codereview.appspot.com/698041/diff/1/18#newcode5 src/compiler/translator_frontend.vcproj:5: Name="translator_frontend" tab http://codereview.appspot.com/698041/show To unsubscribe from this group, send email to angleproject-review+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
Sign in to reply to this message.
http://codereview.appspot.com/698041/diff/9001/10002 File samples/translator/translator.cpp (right): http://codereview.appspot.com/698041/diff/9001/10002#newcode2 samples/translator/translator.cpp:2: // Copyright (c) 2002-2010 The GATE Project Authors. All rights reserved. On 2010/03/24 01:25:20, kbr1 wrote: > GATE -> ANGLE? > Done. http://codereview.appspot.com/698041/diff/9001/10020 File src/compiler/OutputGLSL.cpp (right): http://codereview.appspot.com/698041/diff/9001/10020#newcode2 src/compiler/OutputGLSL.cpp:2: // Copyright (c) 2002-2010 The ANGLE Project Authors. All rights reserved. I am using the same header used in other files. I do not know why we are using 2002-2010 - probably because this code comes from 3dlabs? http://codereview.appspot.com/698041/diff/9001/10020#newcode80 src/compiler/OutputGLSL.cpp:80: //symbol->dump(parseContext.infoSink); On 2010/03/24 01:25:20, kbr1 wrote: > Remove > The header part is still work in progress. I need it for debugging purposes. I promise I will remove it soon :)
Sign in to reply to this message.
On Tue, Mar 23, 2010 at 7:25 PM, <kbr@chromium.org> wrote: > LGTM. Couple of high-level comments. > > 1) What is the plan when we are running on an OpenGL ES 2.0 > implementation? Will we just feed the shader to the implementation or > still at least validate it? Some GLSL ES implementations might be > permissive, so it may be worth validating, for example, that floating > point precision is specified in the fragment shader. Yes preprocessing and validating will still be useful. > 2) How complete is the translation and validation phase currently? Is > some of this code in other files? For example, I don't see any stripping > of precision qualifiers in the GLSL translation pass. As you can see a lot of constructs are UNIMPLEMENTED right now. I am implementing them as I encounter them in a shader. The current version works on all the shaders (except two) under o3d tree. I do not have anything to strip precision qualifiers yet. > 3) Run gcl lint and look through the lint errors. While I guess we > aren't making this code conform to the Google style, there are still > tabs in some of the non-Visual-Studio files, for example. I did not find any tabs in this CL. Please let me know if you know of any file. > 4) Personally I would prefer the naming convention glsl_es_to_... rather > than essl_... but that's a matter of taste. Not a big deal. I can change it if you have a strong preference. I like essl because it is 4 letters as glsl and hlsl. > http://codereview.appspot.com/698041/diff/9001/10002 > File samples/translator/translator.cpp (right): > > http://codereview.appspot.com/698041/diff/9001/10002#newcode2 > samples/translator/translator.cpp:2: // Copyright (c) 2002-2010 The GATE > Project Authors. All rights reserved. > GATE -> ANGLE? > > http://codereview.appspot.com/698041/diff/9001/10020 > File src/compiler/OutputGLSL.cpp (right): > > http://codereview.appspot.com/698041/diff/9001/10020#newcode2 > src/compiler/OutputGLSL.cpp:2: // Copyright (c) 2002-2010 The ANGLE > Project Authors. All rights reserved. > Presumably just 2010? > > http://codereview.appspot.com/698041/diff/9001/10020#newcode80 > src/compiler/OutputGLSL.cpp:80: //symbol->dump(parseContext.infoSink); > Remove > > http://codereview.appspot.com/698041/show >
Sign in to reply to this message.
Hi Nicholas, You're bringing up some good examples where * makes more sense when it stays with the variable name. In my mind the biggest reason for keeping the * with the type is that it is actually integral to the type definition. For example, a pointer to an int is int*. If you were to cast something to a new type you would use static_cast<int*>(foo) . I do realize though that there's valid arguments on both sides. A quick survey of our style guides reveal: - WebKit : T* var - Chromium: T* var - Internal Google style guide: either T* var or T *var but not T * var . Stay consistent within the file. Ideally we should stay consistent throughout the project but given that there's already a lot of code there, unless anyone objects, I'd like to follow the internal google guideline and stay consistent within the file. Vangelis On Tue, Mar 23, 2010 at 7:50 PM, Nicolas Capens <nicolas@transgaming.com>wrote: > Hi Vangelis, > > Regarding sticking the * with the type or variable name: If I may put my > two > cents in, I believe there are strong technical reasons to stick it to the > variable name. Here are five real-world situations where this becomes more > than a style decision: > > 1) A declaration like "int* p, q;" does not declare two pointers, it > declares a pointer and an integer. And it gets really awkward to have to > write "int* p, *q;" when you do want two pointers. By writing "int *p, q;" > and "int *p, *q;" both cases are easy to interpret correctly and > consistent. > > 2) Writing "(int*) p;" results in a compilation error. You would think that > if "int*" represented a valid type in a declaration you could enclose it > with parenthesis. On the other hand "int (*p);" does compile correctly. > > 3) The asterisk can be regarded as a decoration of the name, and without > the > decoration you get the pointer. So when writing something like "const int > **p;", "**p" is the full name of a constant integer, and indeed it can be > used as one. "*p" is the name of a pointer to a constant integer, and "p" > is > the name of a pointer to a pointer to a constant integer. If instead one > would write "const int** p;", something like "*p" has little connection to > the declaration (it's hard to tell whether the address is constant or > whether the integer is constant). > > 4) In C/C++ types are always read from the variable name outward. A > declaration like "int *const &r;" is a reference to a constant pointer to > an > integer. Reading it left-to-right instead and trying to interpret it > correctly is quite hard if not impossible but you'd be encouraged to do > that > if it was written as "int* const& r;". And things can get much, much more > complicated than this. > > 5) Something like "int (*p)[2];" declares a pointer to an array of > integers, > while "int* p[2];" declares an array of pointers. That's something entirely > different. Of course you could write "int(* p)[2];" but that quickly > becomes > nonsensical ascii soup. Likewise the notation for function pointers and > even > more so method pointers demand placing the asterisk next to the variable > name if you want to avoid spaces in the weirdest places. > > Kind regards, > > Nicolas > > > -----Original Message----- > From: angleproject-review@googlegroups.com > [mailto:angleproject-review@googlegroups.com] On Behalf Of > vangelis@google.com > Sent: Tuesday, March 23, 2010 23:34 > To: alokp@chromium.org; daniel@transgaming.com > Cc: angleproject-review@googlegroups.com; reply@codereview.appspotmail.com > Subject: Re: Added GLSL backend for ESSL translator. (issue698041) > > Mostly comments about style issues. > > > http://codereview.appspot.com/698041/diff/1/2 > File samples/samples.sln (right): > > http://codereview.appspot.com/698041/diff/1/2#newcode72 > samples/samples.sln:72: ProjectSection(ProjectDependencies) = > postProject > Was this section hand-edited? I wouldn't expect to see tabs here. > > http://codereview.appspot.com/698041/diff/1/3 > File samples/translator/translator.cpp (right): > > http://codereview.appspot.com/698041/diff/1/3#newcode82 > samples/translator/translator.cpp:82: case 'i': debugOptions |= > EDebugOpIntermediate; break; > there seem to be some issues with the alignment of the case statements > here. > > http://codereview.appspot.com/698041/diff/1/3#newcode101 > samples/translator/translator.cpp:101: if (! CompileFile(argv[0], > compilers[numCompilers-1], debugOptions, &resources)) > remove space after ! > > http://codereview.appspot.com/698041/diff/1/3#newcode180 > samples/translator/translator.cpp:180: printf("Usage: standalone [-i -a > -c -o -m] file1 file2 ...\n" > document the flags here? > > http://codereview.appspot.com/698041/diff/1/3#newcode190 > samples/translator/translator.cpp:190: FILE *in = fopen(fileName, > "r"); > remove extra spaces before = > > http://codereview.appspot.com/698041/diff/1/3#newcode193 > samples/translator/translator.cpp:193: > char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1); > add space after ** > > http://codereview.appspot.com/698041/diff/1/3#newcode206 > samples/translator/translator.cpp:206: if (!(fdata = (char > *)malloc(count+2))) { > remove space between char and * > > http://codereview.appspot.com/698041/diff/1/3#newcode216 > samples/translator/translator.cpp:216: if(count==0){ > I notice in general that the spacing you use in expressions is a bit > inconsistent. Sometimes you use spaces around = sometimes not, etc. > Ideally you should stick with one style. > > http://codereview.appspot.com/698041/diff/1/3#newcode243 > samples/translator/translator.cpp:243: void FreeFileData(char **data) > char** data > > http://codereview.appspot.com/698041/diff/1/3#newcode255 > samples/translator/translator.cpp:255: int ShOutputMultipleStrings(char > **argv) > char** argv > > It's a style issue but it seems that most guides say the * sticks with > the type. > > http://codereview.appspot.com/698041/diff/1/6 > File src/ANGLE.sln (right): > > http://codereview.appspot.com/698041/diff/1/6#newcode22 > src/ANGLE.sln:22: ProjectSection(ProjectDependencies) = postProject > tabs, again. > > http://codereview.appspot.com/698041/diff/1/10 > File src/compiler/intermOut.cpp (right): > > http://codereview.appspot.com/698041/diff/1/10#newcode100 > src/compiler/intermOut.cpp:100: case EOpInitialize: > out.debug << "initialize first child with second child"; break; > watch indentation > > http://codereview.appspot.com/698041/diff/1/18 > File src/compiler/translator_frontend.vcproj (right): > > http://codereview.appspot.com/698041/diff/1/18#newcode5 > src/compiler/translator_frontend.vcproj:5: Name="translator_frontend" > tab > > http://codereview.appspot.com/698041/show > > To unsubscribe from this group, send email to > angleproject-review+unsubscribegooglegroups.com or reply to this email > with > the words "REMOVE ME" as the subject. > > To unsubscribe from this group, send email to angleproject-review+ > unsubscribegooglegroups.com or reply to this email with the words "REMOVE > ME" as the subject. >
Sign in to reply to this message.
Daniel? Any objections before I check it in?
Sign in to reply to this message.
Hi Vangelis, As Nicolas pointed out, we've been using the * aligned with the variable name (at least in the new code we've been adding). We've based our coding style off of the Google C++ coding styles, with some modifications. If desired, I can make these available for you. That said, there are already some differences between our code and the existing 3DLabs code and there will likely be some with the code that you are adding. While it's desirable to minimize differences, I'm not sure that it necessarily makes sense to impose all of our decisions on the areas that you will be maintaining. We certainly agree that staying consistent within a given file/area does trump other coding styles. Thanks, Daniel On 2010-03-24, at 12:53 PM, Vangelis Kokkevis wrote: > Hi Nicholas, > You're bringing up some good examples where * makes more sense when it stays with the variable name. In my mind the biggest reason for keeping the * with the type is that it is actually integral to the type definition. For example, a pointer to an int is int*. If you were to cast something to a new type you would use static_cast<int*>(foo) . I do realize though that there's valid arguments on both sides. A quick survey of our style guides reveal: > > - WebKit : T* var > - Chromium: T* var > - Internal Google style guide: either T* var or T *var but not T * var . Stay consistent within the file. > > Ideally we should stay consistent throughout the project but given that there's already a lot of code there, unless anyone objects, I'd like to follow the internal google guideline and stay consistent within the file. > > Vangelis > > > On Tue, Mar 23, 2010 at 7:50 PM, Nicolas Capens <nicolas@transgaming.com> wrote: > Hi Vangelis, > > Regarding sticking the * with the type or variable name: If I may put my two > cents in, I believe there are strong technical reasons to stick it to the > variable name. Here are five real-world situations where this becomes more > than a style decision: > > 1) A declaration like "int* p, q;" does not declare two pointers, it > declares a pointer and an integer. And it gets really awkward to have to > write "int* p, *q;" when you do want two pointers. By writing "int *p, q;" > and "int *p, *q;" both cases are easy to interpret correctly and consistent. > > 2) Writing "(int*) p;" results in a compilation error. You would think that > if "int*" represented a valid type in a declaration you could enclose it > with parenthesis. On the other hand "int (*p);" does compile correctly. > > 3) The asterisk can be regarded as a decoration of the name, and without the > decoration you get the pointer. So when writing something like "const int > **p;", "**p" is the full name of a constant integer, and indeed it can be > used as one. "*p" is the name of a pointer to a constant integer, and "p" is > the name of a pointer to a pointer to a constant integer. If instead one > would write "const int** p;", something like "*p" has little connection to > the declaration (it's hard to tell whether the address is constant or > whether the integer is constant). > > 4) In C/C++ types are always read from the variable name outward. A > declaration like "int *const &r;" is a reference to a constant pointer to an > integer. Reading it left-to-right instead and trying to interpret it > correctly is quite hard if not impossible but you'd be encouraged to do that > if it was written as "int* const& r;". And things can get much, much more > complicated than this. > > 5) Something like "int (*p)[2];" declares a pointer to an array of integers, > while "int* p[2];" declares an array of pointers. That's something entirely > different. Of course you could write "int(* p)[2];" but that quickly becomes > nonsensical ascii soup. Likewise the notation for function pointers and even > more so method pointers demand placing the asterisk next to the variable > name if you want to avoid spaces in the weirdest places. > > Kind regards, > > Nicolas > > > -----Original Message----- > From: angleproject-review@googlegroups.com > [mailto:angleproject-review@googlegroups.com] On Behalf Of > vangelis@google.com > Sent: Tuesday, March 23, 2010 23:34 > To: alokp@chromium.org; daniel@transgaming.com > Cc: angleproject-review@googlegroups.com; reply@codereview.appspotmail.com > Subject: Re: Added GLSL backend for ESSL translator. (issue698041) > > Mostly comments about style issues. > > > http://codereview.appspot.com/698041/diff/1/2 > File samples/samples.sln (right): > > http://codereview.appspot.com/698041/diff/1/2#newcode72 > samples/samples.sln:72: ProjectSection(ProjectDependencies) = > postProject > Was this section hand-edited? I wouldn't expect to see tabs here. > > http://codereview.appspot.com/698041/diff/1/3 > File samples/translator/translator.cpp (right): > > http://codereview.appspot.com/698041/diff/1/3#newcode82 > samples/translator/translator.cpp:82: case 'i': debugOptions |= > EDebugOpIntermediate; break; > there seem to be some issues with the alignment of the case statements > here. > > http://codereview.appspot.com/698041/diff/1/3#newcode101 > samples/translator/translator.cpp:101: if (! CompileFile(argv[0], > compilers[numCompilers-1], debugOptions, &resources)) > remove space after ! > > http://codereview.appspot.com/698041/diff/1/3#newcode180 > samples/translator/translator.cpp:180: printf("Usage: standalone [-i -a > -c -o -m] file1 file2 ...\n" > document the flags here? > > http://codereview.appspot.com/698041/diff/1/3#newcode190 > samples/translator/translator.cpp:190: FILE *in = fopen(fileName, > "r"); > remove extra spaces before = > > http://codereview.appspot.com/698041/diff/1/3#newcode193 > samples/translator/translator.cpp:193: > char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1); > add space after ** > > http://codereview.appspot.com/698041/diff/1/3#newcode206 > samples/translator/translator.cpp:206: if (!(fdata = (char > *)malloc(count+2))) { > remove space between char and * > > http://codereview.appspot.com/698041/diff/1/3#newcode216 > samples/translator/translator.cpp:216: if(count==0){ > I notice in general that the spacing you use in expressions is a bit > inconsistent. Sometimes you use spaces around = sometimes not, etc. > Ideally you should stick with one style. > > http://codereview.appspot.com/698041/diff/1/3#newcode243 > samples/translator/translator.cpp:243: void FreeFileData(char **data) > char** data > > http://codereview.appspot.com/698041/diff/1/3#newcode255 > samples/translator/translator.cpp:255: int ShOutputMultipleStrings(char > **argv) > char** argv > > It's a style issue but it seems that most guides say the * sticks with > the type. > > http://codereview.appspot.com/698041/diff/1/6 > File src/ANGLE.sln (right): > > http://codereview.appspot.com/698041/diff/1/6#newcode22 > src/ANGLE.sln:22: ProjectSection(ProjectDependencies) = postProject > tabs, again. > > http://codereview.appspot.com/698041/diff/1/10 > File src/compiler/intermOut.cpp (right): > > http://codereview.appspot.com/698041/diff/1/10#newcode100 > src/compiler/intermOut.cpp:100: case EOpInitialize: > out.debug << "initialize first child with second child"; break; > watch indentation > > http://codereview.appspot.com/698041/diff/1/18 > File src/compiler/translator_frontend.vcproj (right): > > http://codereview.appspot.com/698041/diff/1/18#newcode5 > src/compiler/translator_frontend.vcproj:5: Name="translator_frontend" > tab > > http://codereview.appspot.com/698041/show > > To unsubscribe from this group, send email to > angleproject-review+unsubscribegooglegroups.com or reply to this email with > the words "REMOVE ME" as the subject. > > To unsubscribe from this group, send email to angleproject-review+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject. > --- Daniel Koch -+- daniel@transgaming.com Graphics Technology Lead -+- TransGaming Inc. -+- www.transgaming.com
Sign in to reply to this message.
On 2010/03/24 16:52:46, alokp wrote: > I did not find any tabs in this CL. Please let me know if you know of any file. I checked again and the ones I thought I saw aren't there in your current code. > Not a big deal. I can change it if you have a strong preference. I > like essl because it is 4 letters as glsl and hlsl. No strong preference. Regarding the copyright notices, if there are new files (i.e., not based on 3DLabs code) the copyright year should just be 2010.
Sign in to reply to this message.
On Wed, Mar 24, 2010 at 10:30 AM, Daniel Koch <daniel@transgaming.com>wrote: > Hi Vangelis, > > As Nicolas pointed out, we've been using the * aligned with the variable > name (at least in the new code we've been adding). We've based our coding > style off of the Google C++ coding styles, with some modifications. If > desired, I can make these available for you. > > That said, there are already some differences between our code and the > existing 3DLabs code and there will likely be some with the code that you > are adding. While it's desirable to minimize differences, I'm not sure that > it necessarily makes sense to impose all of our decisions on the areas that > you will be maintaining. > > We certainly agree that staying consistent within a given file/area does > trump other coding styles. > Yes, I agree, and sorry if wasn't clear before. I'd like us to stay consistent within the file boundaries (which is what the internal google guidelines suggest). Also, no need to go and fix third party code either. As for the coding style standard you're using, it would be worth putting it up on the Wiki at some point to serve as a guideline for other contributors. Thanks, Vangelis > > Thanks, > Daniel > > On 2010-03-24, at 12:53 PM, Vangelis Kokkevis wrote: > > Hi Nicholas, > You're bringing up some good examples where * makes more sense when it > stays with the variable name. In my mind the biggest reason for keeping the > * with the type is that it is actually integral to the type definition. For > example, a pointer to an int is int*. If you were to cast something to a > new type you would use static_cast<int*>(foo) . I do realize though that > there's valid arguments on both sides. A quick survey of our style guides > reveal: > > - WebKit : T* var > - Chromium: T* var > - Internal Google style guide: either T* var or T *var but not T * var . > Stay consistent within the file. > > Ideally we should stay consistent throughout the project but given that > there's already a lot of code there, unless anyone objects, I'd like to > follow the internal google guideline and stay consistent within the file. > > Vangelis > > > On Tue, Mar 23, 2010 at 7:50 PM, Nicolas Capens <nicolas@transgaming.com>wrote: > >> Hi Vangelis, >> >> Regarding sticking the * with the type or variable name: If I may put my >> two >> cents in, I believe there are strong technical reasons to stick it to the >> variable name. Here are five real-world situations where this becomes more >> than a style decision: >> >> 1) A declaration like "int* p, q;" does not declare two pointers, it >> declares a pointer and an integer. And it gets really awkward to have to >> write "int* p, *q;" when you do want two pointers. By writing "int *p, q;" >> and "int *p, *q;" both cases are easy to interpret correctly and >> consistent. >> >> 2) Writing "(int*) p;" results in a compilation error. You would think >> that >> if "int*" represented a valid type in a declaration you could enclose it >> with parenthesis. On the other hand "int (*p);" does compile correctly. >> >> 3) The asterisk can be regarded as a decoration of the name, and without >> the >> decoration you get the pointer. So when writing something like "const int >> **p;", "**p" is the full name of a constant integer, and indeed it can be >> used as one. "*p" is the name of a pointer to a constant integer, and "p" >> is >> the name of a pointer to a pointer to a constant integer. If instead one >> would write "const int** p;", something like "*p" has little connection to >> the declaration (it's hard to tell whether the address is constant or >> whether the integer is constant). >> >> 4) In C/C++ types are always read from the variable name outward. A >> declaration like "int *const &r;" is a reference to a constant pointer to >> an >> integer. Reading it left-to-right instead and trying to interpret it >> correctly is quite hard if not impossible but you'd be encouraged to do >> that >> if it was written as "int* const& r;". And things can get much, much more >> complicated than this. >> >> 5) Something like "int (*p)[2];" declares a pointer to an array of >> integers, >> while "int* p[2];" declares an array of pointers. That's something >> entirely >> different. Of course you could write "int(* p)[2];" but that quickly >> becomes >> nonsensical ascii soup. Likewise the notation for function pointers and >> even >> more so method pointers demand placing the asterisk next to the variable >> name if you want to avoid spaces in the weirdest places. >> >> Kind regards, >> >> Nicolas >> >> >> -----Original Message----- >> From: angleproject-review@googlegroups.com >> [mailto:angleproject-review@googlegroups.com] On Behalf Of >> vangelis@google.com >> Sent: Tuesday, March 23, 2010 23:34 >> To: alokp@chromium.org; daniel@transgaming.com >> Cc: angleproject-review@googlegroups.com; >> reply@codereview.appspotmail.com >> Subject: Re: Added GLSL backend for ESSL translator. (issue698041) >> >> Mostly comments about style issues. >> >> >> http://codereview.appspot.com/698041/diff/1/2 >> File samples/samples.sln (right): >> >> http://codereview.appspot.com/698041/diff/1/2#newcode72 >> samples/samples.sln:72<http://codereview.appspot.com/698041/diff/1/2#newcode72samples/samples.sln:72>: >> ProjectSection(ProjectDependencies) = >> postProject >> Was this section hand-edited? I wouldn't expect to see tabs here. >> >> http://codereview.appspot.com/698041/diff/1/3 >> File samples/translator/translator.cpp (right): >> >> http://codereview.appspot.com/698041/diff/1/3#newcode82 >> samples/translator/translator.cpp:82<http://codereview.appspot.com/698041/diff/1/3#newcode82samples/translator/translator.cpp:82>: >> case 'i': debugOptions |= >> EDebugOpIntermediate; break; >> there seem to be some issues with the alignment of the case statements >> here. >> >> http://codereview.appspot.com/698041/diff/1/3#newcode101 >> samples/translator/translator.cpp:101<http://codereview.appspot.com/698041/diff/1/3#newcode101samples/translator/translator.cpp:101>: >> if (! CompileFile(argv[0], >> compilers[numCompilers-1], debugOptions, &resources)) >> remove space after ! >> >> http://codereview.appspot.com/698041/diff/1/3#newcode180 >> samples/translator/translator.cpp:180<http://codereview.appspot.com/698041/diff/1/3#newcode180samples/translator/translator.cpp:180>: >> printf("Usage: standalone [-i -a >> -c -o -m] file1 file2 ...\n" >> document the flags here? >> >> http://codereview.appspot.com/698041/diff/1/3#newcode190 >> samples/translator/translator.cpp:190<http://codereview.appspot.com/698041/diff/1/3#newcode190samples/translator/translator.cpp:190>: >> FILE *in = fopen(fileName, >> "r"); >> remove extra spaces before = >> >> http://codereview.appspot.com/698041/diff/1/3#newcode193 >> samples/translator/translator.cpp:193<http://codereview.appspot.com/698041/diff/1/3#newcode193samples/translator/translator.cpp:193> >> : >> char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1); >> add space after ** >> >> http://codereview.appspot.com/698041/diff/1/3#newcode206 >> samples/translator/translator.cpp:206<http://codereview.appspot.com/698041/diff/1/3#newcode206samples/translator/translator.cpp:206>: >> if (!(fdata = (char >> *)malloc(count+2))) { >> remove space between char and * >> >> http://codereview.appspot.com/698041/diff/1/3#newcode216 >> samples/translator/translator.cpp:216<http://codereview.appspot.com/698041/diff/1/3#newcode216samples/translator/translator.cpp:216>: >> if(count==0){ >> I notice in general that the spacing you use in expressions is a bit >> inconsistent. Sometimes you use spaces around = sometimes not, etc. >> Ideally you should stick with one style. >> >> http://codereview.appspot.com/698041/diff/1/3#newcode243 >> samples/translator/translator.cpp:243<http://codereview.appspot.com/698041/diff/1/3#newcode243samples/translator/translator.cpp:243>: >> void FreeFileData(char **data) >> char** data >> >> http://codereview.appspot.com/698041/diff/1/3#newcode255 >> samples/translator/translator.cpp:255<http://codereview.appspot.com/698041/diff/1/3#newcode255samples/translator/translator.cpp:255>: >> int ShOutputMultipleStrings(char >> **argv) >> char** argv >> >> It's a style issue but it seems that most guides say the * sticks with >> the type. >> >> http://codereview.appspot.com/698041/diff/1/6 >> File src/ANGLE.sln (right): >> >> http://codereview.appspot.com/698041/diff/1/6#newcode22 >> src/ANGLE.sln:22<http://codereview.appspot.com/698041/diff/1/6#newcode22src/ANGLE.sln:22>: >> ProjectSection(ProjectDependencies) = postProject >> tabs, again. >> >> http://codereview.appspot.com/698041/diff/1/10 >> File src/compiler/intermOut.cpp (right): >> >> http://codereview.appspot.com/698041/diff/1/10#newcode100 >> src/compiler/intermOut.cpp:100<http://codereview.appspot.com/698041/diff/1/10#newcode100src/compiler/intermOut.cpp:100>: >> case EOpInitialize: >> out.debug << "initialize first child with second child"; break; >> watch indentation >> >> http://codereview.appspot.com/698041/diff/1/18 >> File src/compiler/translator_frontend.vcproj (right): >> >> http://codereview.appspot.com/698041/diff/1/18#newcode5 >> src/compiler/translator_frontend.vcproj:5<http://codereview.appspot.com/698041/diff/1/18#newcode5src/compiler/translator_frontend.vcproj:5>: >> Name="translator_frontend" >> tab >> >> http://codereview.appspot.com/698041/show >> >> To unsubscribe from this group, send email to >> angleproject-review+unsubscribegooglegroups.com or reply to this email >> with >> the words "REMOVE ME" as the subject. >> >> To unsubscribe from this group, send email to angleproject-review+ >> unsubscribegooglegroups.com or reply to this email with the words "REMOVE >> ME" as the subject. >> > > > --- > Daniel Koch -+- daniel@transgaming.com > Graphics Technology Lead -+- TransGaming Inc. -+- www.transgaming.com > >
Sign in to reply to this message.
Looks good. Daniel
Sign in to reply to this message.
Hi Vangelis, Again this is just my two cents, but I believe comparing it directly to casting is incorrect. For example to cast something to a pointer to an array of integers you could write "static_cast<int(*)[]>(p)". Yet something like "int(*)[] p;" is not a valid declaration. You have to write "int (*p)[];" instead. So writing "int* p;" because "static_cast<int*>(p)" happens to be valid is not a good argument in my humble opinion. Actually the full declaration syntax would be "int (*p);" and the cast is "static_cast<int (*)>(p)". Reading the type from the innermost parenthesis outward (first to the right then to the left) always leads to the correct interpretation. But (thankfully) C allows to drop superfluous parenthesis and spaces so you can safely write "static_cast<int*>(p)", without risk of misinterpretation. For declarations though sticking the asterisk to the variable name is the most consistent notation, that also aids interpretation. Just think of "int (*p)[];" and then removing the array so you end up with "int *p;". I fully understand that "int *p;" looks a bit alien at first but clearly because of the above (and the five arguments I gave previously) C/C++ wants us to write declarations this way. The reasons behind its 'inside-out' type declaration are historical [1]. We can't change the language, so we might as well make sure it is used as intended by its design to avoid any further confusion. I have to add that after adopting this notation my understanding of pointer syntax and semantic, probably the most complex concept in C, improved dramatically. This is just a personal experience and opinion, but I hope I've given enough technical arguments to clarify why this could be significant. Anyway, since other Google project already adopted the other notation I can see why you would want to adopt that as well for consistency reasons. I hope it's not out of place though to warn that several decades later a lot of Microsoft code is still using (Systems) Hungarian notation even though all their naming conventions now explicitly advise against it [2]... Kind regards, Nicolas [1] http://www.peoi.org/Courses/Coursesen/cprog/fram1.html [2] http://www.joelonsoftware.com/articles/Wrong.html From: angleproject-review@googlegroups.com [mailto:angleproject-review@googlegroups.com] On Behalf Of Vangelis Kokkevis Sent: Wednesday, March 24, 2010 17:53 To: angleproject-review@googlegroups.com Cc: alokp@chromium.org; daniel@transgaming.com; reply@codereview.appspotmail.com Subject: Re: Added GLSL backend for ESSL translator. (issue698041) Hi Nicholas, You're bringing up some good examples where * makes more sense when it stays with the variable name. In my mind the biggest reason for keeping the * with the type is that it is actually integral to the type definition. For example, a pointer to an int is int*. If you were to cast something to a new type you would use static_cast<int*>(foo) . I do realize though that there's valid arguments on both sides. A quick survey of our style guides reveal: - WebKit : T* var - Chromium: T* var - Internal Google style guide: either T* var or T *var but not T * var . Stay consistent within the file. Ideally we should stay consistent throughout the project but given that there's already a lot of code there, unless anyone objects, I'd like to follow the internal google guideline and stay consistent within the file. Vangelis On Tue, Mar 23, 2010 at 7:50 PM, Nicolas Capens <nicolas@transgaming.com> wrote: Hi Vangelis, Regarding sticking the * with the type or variable name: If I may put my two cents in, I believe there are strong technical reasons to stick it to the variable name. Here are five real-world situations where this becomes more than a style decision: 1) A declaration like "int* p, q;" does not declare two pointers, it declares a pointer and an integer. And it gets really awkward to have to write "int* p, *q;" when you do want two pointers. By writing "int *p, q;" and "int *p, *q;" both cases are easy to interpret correctly and consistent. 2) Writing "(int*) p;" results in a compilation error. You would think that if "int*" represented a valid type in a declaration you could enclose it with parenthesis. On the other hand "int (*p);" does compile correctly. 3) The asterisk can be regarded as a decoration of the name, and without the decoration you get the pointer. So when writing something like "const int **p;", "**p" is the full name of a constant integer, and indeed it can be used as one. "*p" is the name of a pointer to a constant integer, and "p" is the name of a pointer to a pointer to a constant integer. If instead one would write "const int** p;", something like "*p" has little connection to the declaration (it's hard to tell whether the address is constant or whether the integer is constant). 4) In C/C++ types are always read from the variable name outward. A declaration like "int *const &r;" is a reference to a constant pointer to an integer. Reading it left-to-right instead and trying to interpret it correctly is quite hard if not impossible but you'd be encouraged to do that if it was written as "int* const& r;". And things can get much, much more complicated than this. 5) Something like "int (*p)[2];" declares a pointer to an array of integers, while "int* p[2];" declares an array of pointers. That's something entirely different. Of course you could write "int(* p)[2];" but that quickly becomes nonsensical ascii soup. Likewise the notation for function pointers and even more so method pointers demand placing the asterisk next to the variable name if you want to avoid spaces in the weirdest places. Kind regards, Nicolas -----Original Message----- From: angleproject-review@googlegroups.com [mailto:angleproject-review@googlegroups.com] On Behalf Of vangelis@google.com Sent: Tuesday, March 23, 2010 23:34 To: alokp@chromium.org; daniel@transgaming.com Cc: angleproject-review@googlegroups.com; reply@codereview.appspotmail.com Subject: Re: Added GLSL backend for ESSL translator. (issue698041) Mostly comments about style issues. http://codereview.appspot.com/698041/diff/1/2 File samples/samples.sln (right): http://codereview.appspot.com/698041/diff/1/2#newcode72 <http://codereview.appspot.com/698041/diff/1/2#newcode72samples/samples.sln: 72> samples/samples.sln:72: ProjectSection(ProjectDependencies) = postProject Was this section hand-edited? I wouldn't expect to see tabs here. http://codereview.appspot.com/698041/diff/1/3 File samples/translator/translator.cpp (right): http://codereview.appspot.com/698041/diff/1/3#newcode82 <http://codereview.appspot.com/698041/diff/1/3#newcode82samples/translator/t ranslator.cpp:82> samples/translator/translator.cpp:82: case 'i': debugOptions |= EDebugOpIntermediate; break; there seem to be some issues with the alignment of the case statements here. http://codereview.appspot.com/698041/diff/1/3#newcode101 <http://codereview.appspot.com/698041/diff/1/3#newcode101samples/translator/ translator.cpp:101> samples/translator/translator.cpp:101: if (! CompileFile(argv[0], compilers[numCompilers-1], debugOptions, &resources)) remove space after ! http://codereview.appspot.com/698041/diff/1/3#newcode180 <http://codereview.appspot.com/698041/diff/1/3#newcode180samples/translator/ translator.cpp:180> samples/translator/translator.cpp:180: printf("Usage: standalone [-i -a -c -o -m] file1 file2 ...\n" document the flags here? http://codereview.appspot.com/698041/diff/1/3#newcode190 <http://codereview.appspot.com/698041/diff/1/3#newcode190samples/translator/ translator.cpp:190> samples/translator/translator.cpp:190: FILE *in = fopen(fileName, "r"); remove extra spaces before = http://codereview.appspot.com/698041/diff/1/3#newcode193 <http://codereview.appspot.com/698041/diff/1/3#newcode193samples/translator/ translator.cpp:193> samples/translator/translator.cpp:193: char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1); add space after ** http://codereview.appspot.com/698041/diff/1/3#newcode206 <http://codereview.appspot.com/698041/diff/1/3#newcode206samples/translator/ translator.cpp:206> samples/translator/translator.cpp:206: if (!(fdata = (char *)malloc(count+2))) { remove space between char and * http://codereview.appspot.com/698041/diff/1/3#newcode216 <http://codereview.appspot.com/698041/diff/1/3#newcode216samples/translator/ translator.cpp:216> samples/translator/translator.cpp:216: if(count==0){ I notice in general that the spacing you use in expressions is a bit inconsistent. Sometimes you use spaces around = sometimes not, etc. Ideally you should stick with one style. http://codereview.appspot.com/698041/diff/1/3#newcode243 <http://codereview.appspot.com/698041/diff/1/3#newcode243samples/translator/ translator.cpp:243> samples/translator/translator.cpp:243: void FreeFileData(char **data) char** data http://codereview.appspot.com/698041/diff/1/3#newcode255 <http://codereview.appspot.com/698041/diff/1/3#newcode255samples/translator/ translator.cpp:255> samples/translator/translator.cpp:255: int ShOutputMultipleStrings(char **argv) char** argv It's a style issue but it seems that most guides say the * sticks with the type. http://codereview.appspot.com/698041/diff/1/6 File src/ANGLE.sln (right): http://codereview.appspot.com/698041/diff/1/6#newcode22 <http://codereview.appspot.com/698041/diff/1/6#newcode22src/ANGLE.sln:22> src/ANGLE.sln:22: ProjectSection(ProjectDependencies) = postProject tabs, again. http://codereview.appspot.com/698041/diff/1/10 File src/compiler/intermOut.cpp (right): http://codereview.appspot.com/698041/diff/1/10#newcode100 <http://codereview.appspot.com/698041/diff/1/10#newcode100src/compiler/inter mOut.cpp:100> src/compiler/intermOut.cpp:100: case EOpInitialize: out.debug << "initialize first child with second child"; break; watch indentation http://codereview.appspot.com/698041/diff/1/18 File src/compiler/translator_frontend.vcproj (right): http://codereview.appspot.com/698041/diff/1/18#newcode5 <http://codereview.appspot.com/698041/diff/1/18#newcode5src/compiler/transla tor_frontend.vcproj:5> src/compiler/translator_frontend.vcproj:5: Name="translator_frontend" tab http://codereview.appspot.com/698041/show To unsubscribe from this group, send email to angleproject-review+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject. To unsubscribe from this group, send email to angleproject-review+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject. To unsubscribe from this group, send email to angleproject-review+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
Sign in to reply to this message.
On Thu, Mar 25, 2010 at 4:54 AM, Nicolas Capens <nicolas@transgaming.com>wrote: > Hi Vangelis, > > > > Again this is just my two cents, but I believe comparing it directly to > casting is incorrect. > > > > For example to cast something to a pointer to an array of integers you > could write "static_cast<int(*)[]>(p)". Yet something like "int(*)[] p;" is > not a valid declaration. You have to write "int (*p)[];" instead. So writing > "int* p;" because "static_cast<int*>(p)" happens to be valid is not a good > argument in my humble opinion. > > > > Actually the full declaration syntax would be "int (*p);" and the cast is > "static_cast<int (*)>(p)". Reading the type from the innermost parenthesis > outward (first to the right then to the left) always leads to the correct > interpretation. But (thankfully) C allows to drop superfluous parenthesis > and spaces so you can safely write "static_cast<int*>(p)", without risk of > misinterpretation. For declarations though sticking the asterisk to the > variable name is the most consistent notation, that also aids > interpretation. Just think of "int (*p)[];" and then removing the array so > you end up with "int *p;". > > > > I fully understand that "int *p;" looks a bit alien at first but clearly > because of the above (and the five arguments I gave previously) C/C++ wants > us to write declarations this way. The reasons behind its 'inside-out' type > declaration are historical [1]. We can't change the language, so we might as > well make sure it is used as intended by its design to avoid any further > confusion. > > > > I have to add that after adopting this notation my understanding of pointer > syntax and semantic, probably the most complex concept in C, improved > dramatically. This is just a personal experience and opinion, but I hope > I've given enough technical arguments to clarify why this could be > significant. > > > > Anyway, since other Google project already adopted the other notation I can > see why you would want to adopt that as well for consistency reasons. I hope > it's not out of place though to warn that several decades later a lot of > Microsoft code is still using (Systems) Hungarian notation even though all > their naming conventions now explicitly advise against it [2]... > :) The internal google styleguide also forces an 80 char limit on lines so it's not exactly the bible of modern programming! Feel free to use whatever style feels good to your team. Vangelis > > > Kind regards, > > > > Nicolas > > > > [1] http://www.peoi.org/Courses/Coursesen/cprog/fram1.html > > [2] http://www.joelonsoftware.com/articles/Wrong.html > > > > > > *From:* angleproject-review@googlegroups.com [mailto: > angleproject-review@googlegroups.com] *On Behalf Of *Vangelis Kokkevis > *Sent:* Wednesday, March 24, 2010 17:53 > *To:* angleproject-review@googlegroups.com > *Cc:* alokp@chromium.org; daniel@transgaming.com; > reply@codereview.appspotmail.com > > *Subject:* Re: Added GLSL backend for ESSL translator. (issue698041) > > > > Hi Nicholas, > > You're bringing up some good examples where * makes more sense when it > stays with the variable name. In my mind the biggest reason for keeping the > * with the type is that it is actually integral to the type definition. For > example, a pointer to an int is int*. If you were to cast something to a > new type you would use static_cast<int*>(foo) . I do realize though that > there's valid arguments on both sides. A quick survey of our style guides > reveal: > > > > - WebKit : T* var > > - Chromium: T* var > > - Internal Google style guide: either T* var or T *var but not T * var . > Stay consistent within the file. > > > > Ideally we should stay consistent throughout the project but given that > there's already a lot of code there, unless anyone objects, I'd like to > follow the internal google guideline and stay consistent within the file. > > > > Vangelis > > > > > > On Tue, Mar 23, 2010 at 7:50 PM, Nicolas Capens <nicolas@transgaming.com> > wrote: > > Hi Vangelis, > > Regarding sticking the * with the type or variable name: If I may put my > two > cents in, I believe there are strong technical reasons to stick it to the > variable name. Here are five real-world situations where this becomes more > than a style decision: > > 1) A declaration like "int* p, q;" does not declare two pointers, it > declares a pointer and an integer. And it gets really awkward to have to > write "int* p, *q;" when you do want two pointers. By writing "int *p, q;" > and "int *p, *q;" both cases are easy to interpret correctly and > consistent. > > 2) Writing "(int*) p;" results in a compilation error. You would think that > if "int*" represented a valid type in a declaration you could enclose it > with parenthesis. On the other hand "int (*p);" does compile correctly. > > 3) The asterisk can be regarded as a decoration of the name, and without > the > decoration you get the pointer. So when writing something like "const int > **p;", "**p" is the full name of a constant integer, and indeed it can be > used as one. "*p" is the name of a pointer to a constant integer, and "p" > is > the name of a pointer to a pointer to a constant integer. If instead one > would write "const int** p;", something like "*p" has little connection to > the declaration (it's hard to tell whether the address is constant or > whether the integer is constant). > > 4) In C/C++ types are always read from the variable name outward. A > declaration like "int *const &r;" is a reference to a constant pointer to > an > integer. Reading it left-to-right instead and trying to interpret it > correctly is quite hard if not impossible but you'd be encouraged to do > that > if it was written as "int* const& r;". And things can get much, much more > complicated than this. > > 5) Something like "int (*p)[2];" declares a pointer to an array of > integers, > while "int* p[2];" declares an array of pointers. That's something entirely > different. Of course you could write "int(* p)[2];" but that quickly > becomes > nonsensical ascii soup. Likewise the notation for function pointers and > even > more so method pointers demand placing the asterisk next to the variable > name if you want to avoid spaces in the weirdest places. > > Kind regards, > > Nicolas > > > > -----Original Message----- > From: angleproject-review@googlegroups.com > [mailto:angleproject-review@googlegroups.com] On Behalf Of > vangelis@google.com > Sent: Tuesday, March 23, 2010 23:34 > To: alokp@chromium.org; daniel@transgaming.com > Cc: angleproject-review@googlegroups.com; reply@codereview.appspotmail.com > Subject: Re: Added GLSL backend for ESSL translator. (issue698041) > > Mostly comments about style issues. > > > http://codereview.appspot.com/698041/diff/1/2 > File samples/samples.sln (right): > > http://codereview.appspot.com/698041/diff/1/2#newcode72 > samples/samples.sln:72<http://codereview.appspot.com/698041/diff/1/2#newcode72%0Dsamples/samples.sln:72>: > ProjectSection(ProjectDependencies) = > postProject > Was this section hand-edited? I wouldn't expect to see tabs here. > > http://codereview.appspot.com/698041/diff/1/3 > File samples/translator/translator.cpp (right): > > http://codereview.appspot.com/698041/diff/1/3#newcode82 > samples/translator/translator.cpp:82<http://codereview.appspot.com/698041/diff/1/3#newcode82%0Dsamples/translator/translator.cpp:82>: > case 'i': debugOptions |= > EDebugOpIntermediate; break; > there seem to be some issues with the alignment of the case statements > here. > > http://codereview.appspot.com/698041/diff/1/3#newcode101 > samples/translator/translator.cpp:101<http://codereview.appspot.com/698041/diff/1/3#newcode101%0Dsamples/translator/translator.cpp:101>: > if (! CompileFile(argv[0], > compilers[numCompilers-1], debugOptions, &resources)) > remove space after ! > > http://codereview.appspot.com/698041/diff/1/3#newcode180 > samples/translator/translator.cpp:180<http://codereview.appspot.com/698041/diff/1/3#newcode180%0Dsamples/translator/translator.cpp:180>: > printf("Usage: standalone [-i -a > -c -o -m] file1 file2 ...\n" > document the flags here? > > http://codereview.appspot.com/698041/diff/1/3#newcode190 > samples/translator/translator.cpp:190<http://codereview.appspot.com/698041/diff/1/3#newcode190%0Dsamples/translator/translator.cpp:190>: > FILE *in = fopen(fileName, > "r"); > remove extra spaces before = > > http://codereview.appspot.com/698041/diff/1/3#newcode193 > samples/translator/translator.cpp:193<http://codereview.appspot.com/698041/diff/1/3#newcode193%0Dsamples/translator/translator.cpp:193> > : > char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1); > add space after ** > > http://codereview.appspot.com/698041/diff/1/3#newcode206 > samples/translator/translator.cpp:206<http://codereview.appspot.com/698041/diff/1/3#newcode206%0Dsamples/translator/translator.cpp:206>: > if (!(fdata = (char > *)malloc(count+2))) { > remove space between char and * > > http://codereview.appspot.com/698041/diff/1/3#newcode216 > samples/translator/translator.cpp:216<http://codereview.appspot.com/698041/diff/1/3#newcode216%0Dsamples/translator/translator.cpp:216>: > if(count==0){ > I notice in general that the spacing you use in expressions is a bit > inconsistent. Sometimes you use spaces around = sometimes not, etc. > Ideally you should stick with one style. > > http://codereview.appspot.com/698041/diff/1/3#newcode243 > samples/translator/translator.cpp:243<http://codereview.appspot.com/698041/diff/1/3#newcode243%0Dsamples/translator/translator.cpp:243>: > void FreeFileData(char **data) > char** data > > http://codereview.appspot.com/698041/diff/1/3#newcode255 > samples/translator/translator.cpp:255<http://codereview.appspot.com/698041/diff/1/3#newcode255%0Dsamples/translator/translator.cpp:255>: > int ShOutputMultipleStrings(char > **argv) > char** argv > > It's a style issue but it seems that most guides say the * sticks with > the type. > > http://codereview.appspot.com/698041/diff/1/6 > File src/ANGLE.sln (right): > > http://codereview.appspot.com/698041/diff/1/6#newcode22 > src/ANGLE.sln:22<http://codereview.appspot.com/698041/diff/1/6#newcode22%0Dsrc/ANGLE.sln:22>: > ProjectSection(ProjectDependencies) = postProject > tabs, again. > > http://codereview.appspot.com/698041/diff/1/10 > File src/compiler/intermOut.cpp (right): > > http://codereview.appspot.com/698041/diff/1/10#newcode100 > src/compiler/intermOut.cpp:100<http://codereview.appspot.com/698041/diff/1/10#newcode100%0Dsrc/compiler/intermOut.cpp:100>: > case EOpInitialize: > out.debug << "initialize first child with second child"; break; > watch indentation > > http://codereview.appspot.com/698041/diff/1/18 > File src/compiler/translator_frontend.vcproj (right): > > http://codereview.appspot.com/698041/diff/1/18#newcode5 > src/compiler/translator_frontend.vcproj:5<http://codereview.appspot.com/698041/diff/1/18#newcode5%0Dsrc/compiler/translator_frontend.vcproj:5>: > Name="translator_frontend" > tab > > http://codereview.appspot.com/698041/show > > To unsubscribe from this group, send email to > angleproject-review+unsubscribegooglegroups.com or reply to this email > with > the words "REMOVE ME" as the subject. > > To unsubscribe from this group, send email to angleproject-review+ > unsubscribegooglegroups.com or reply to this email with the words "REMOVE > ME" as the subject. > > > > To unsubscribe from this group, send email to angleproject-review+ > unsubscribegooglegroups.com or reply to this email with the words "REMOVE > ME" as the subject. > > To unsubscribe from this group, send email to angleproject-review+ > unsubscribegooglegroups.com or reply to this email with the words "REMOVE > ME" as the subject. >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
