Issue 698041: Added GLSL backend for ESSL translator.

vangelis

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

14 years, 8 months ago (2010-03-23 22:34:28 UTC) #2

Alok Priyadarshi

what do you guys think about using translator in project names? will it be confusing ...

14 years, 8 months ago (2010-03-23 23:24:51 UTC) #3

vangelis

I think "translator" is a good name for what we're trying to do. http://codereview.appspot.com/698041/diff/1/2 File ...

14 years, 8 months ago (2010-03-23 23:45:11 UTC) #5

kbr1

LGTM. Couple of high-level comments. 1) What is the plan when we are running on ...

14 years, 8 months ago (2010-03-24 01:25:20 UTC) #6

kbr1

(Resending just to get archived in angleproject-review) On 2010/03/24 01:25:20, kbr1 wrote: > LGTM. Couple ...

14 years, 8 months ago (2010-03-24 01:33:26 UTC) #7

nicolas

Hi Vangelis, Regarding sticking the * with the type or variable name: If I may ...

14 years, 8 months ago (2010-03-24 02:50:19 UTC) #8

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.

Alok Priyadarshi

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

14 years, 8 months ago (2010-03-24 16:46:49 UTC) #9

Alok Priyadarshi

On Tue, Mar 23, 2010 at 7:25 PM, <kbr@chromium.org> wrote: > LGTM. Couple of high-level ...

14 years, 8 months ago (2010-03-24 16:52:46 UTC) #10

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.

vangelis

Hi Nicholas, You're bringing up some good examples where * makes more sense when it ...

14 years, 8 months ago (2010-03-24 16:53:29 UTC) #11

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.

dgkoch

Hi Vangelis, As Nicolas pointed out, we've been using the * aligned with the variable ...

14 years, 8 months ago (2010-03-24 17:31:01 UTC) #13

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.

kbr1

On 2010/03/24 16:52:46, alokp wrote: > I did not find any tabs in this CL. ...

14 years, 8 months ago (2010-03-24 17:40:30 UTC) #14

vangelis

On Wed, Mar 24, 2010 at 10:30 AM, Daniel Koch <daniel@transgaming.com>wrote: > Hi Vangelis, > ...

14 years, 8 months ago (2010-03-24 17:45:20 UTC) #15

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.

nicolas

Hi Vangelis, Again this is just my two cents, but I believe comparing it directly ...

14 years, 8 months ago (2010-03-25 11:54:47 UTC) #17

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.

vangelis

14 years, 8 months ago (2010-03-25 16:17:15 UTC) #18

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.

Issue 698041: Added GLSL backend for ESSL translator. (Closed)

Description

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Messages