[Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
From: Kevin Rogovin kevin.rogo...@intel.com
Define the infrastructure for the extension GL_ARB_framebuffer_no_attachments:
- extension table
- additions to gl_framebuffer
v1 - v2
Spacing and trailing spaces fixes.
v2 - v3
mtypes.h: Correct comment on _HasAttachments.
Signed-off-by: Kevin Rogovin kevin.rogo...@intel.com
---
src/mesa/main/extensions.c | 1 +
src/mesa/main/fbobject.c| 1 +
src/mesa/main/framebuffer.c | 1 +
src/mesa/main/mtypes.h | 50 -
4 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/src/mesa/main/extensions.c b/src/mesa/main/extensions.c
index c82416a..3256b2c 100644
--- a/src/mesa/main/extensions.c
+++ b/src/mesa/main/extensions.c
@@ -117,6 +117,7 @@ static const struct extension extension_table[] = {
{ GL_ARB_fragment_program,o(ARB_fragment_program),
GLL,2002 },
{ GL_ARB_fragment_program_shadow,
o(ARB_fragment_program_shadow), GLL,2003 },
{ GL_ARB_fragment_shader, o(ARB_fragment_shader),
GL, 2002 },
+ { GL_ARB_framebuffer_no_attachments,
o(ARB_framebuffer_no_attachments), GL, 2012 },
{ GL_ARB_framebuffer_object, o(ARB_framebuffer_object),
GL, 2005 },
{ GL_ARB_framebuffer_sRGB,o(EXT_framebuffer_sRGB),
GL, 1998 },
{ GL_ARB_get_program_binary, o(dummy_true),
GL, 2010 },
diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
index 1859c27..8fea7f8 100644
--- a/src/mesa/main/fbobject.c
+++ b/src/mesa/main/fbobject.c
@@ -957,6 +957,7 @@ _mesa_test_framebuffer_completeness(struct gl_context *ctx,
fb-Height = 0;
fb-_AllColorBuffersFixedPoint = GL_TRUE;
fb-_HasSNormOrFloatColorBuffer = GL_FALSE;
+ fb-_HasAttachments = GL_TRUE;
/* Start at -2 to more easily loop over all attachment points.
* -2: depth buffer
diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
index 665a5ba..c2cfb92 100644
--- a/src/mesa/main/framebuffer.c
+++ b/src/mesa/main/framebuffer.c
@@ -157,6 +157,7 @@ _mesa_initialize_window_framebuffer(struct gl_framebuffer
*fb,
fb-_Status = GL_FRAMEBUFFER_COMPLETE_EXT;
fb-_AllColorBuffersFixedPoint = !visual-floatMode;
fb-_HasSNormOrFloatColorBuffer = visual-floatMode;
+ fb-_HasAttachments = GL_TRUE;
compute_depth_max(fb);
}
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 8342517..1a37aa6 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -3146,12 +3146,29 @@ struct gl_framebuffer
*/
struct gl_config Visual;
- GLuint Width, Height; /** size of frame buffer in pixels */
+ /**
+* size of frame buffer in pixels,
+* no attachments has these values as 0
+*/
+ GLuint Width, Height;
+
+ /**
+* In the case that the framebuffer has no attachment (i.e.
+* GL_ARB_framebuffer_no_attachments) then the geometry of
+* the framebuffer is specified by the default values.
+*/
+ struct {
+ GLuint Width, Height, Layers, NumSamples;
+ GLboolean FixedSampleLocations;
+ } DefaultGeometry;
- /** \name Drawing bounds (Intersection of buffer size and scissor box) */
+ /** \name Drawing bounds (Intersection of buffer size and scissor box)
+* The drawing region is given by [_Xmin, _Xmax) x [_Ymin, _Ymax),
+* (inclusive for _Xmin and _Ymin while exclusive for _Xmax and _Ymax)
+*/
/*@{*/
- GLint _Xmin, _Xmax; /** inclusive */
- GLint _Ymin, _Ymax; /** exclusive */
+ GLint _Xmin, _Xmax;
+ GLint _Ymin, _Ymax;
/*@}*/
/** \name Derived Z buffer stuff */
@@ -3164,6 +3181,18 @@ struct gl_framebuffer
/** One of the GL_FRAMEBUFFER_(IN)COMPLETE_* tokens */
GLenum _Status;
+ /** True if one of Attachment has Type != GL_NONE
+* NOTE: the values for Width and Height are set to 0 in
+* case of no attachments, a backend driver supporting
+* GL_ARB_framebuffer_no_attachments must check for the
+* flag _HasAttachments and if GL_FALSE, must then use
+* the values in DefaultGeometry to initialize its
+* viewport, scissor and so on (in particular _Xmin,
+* _Xmax, _Ymin and _Ymax do NOT take into account
+* _HasAttachments being false)
+*/
+ GLboolean _HasAttachments;
+
/** Integer color values */
GLboolean _IntegerColor;
@@ -3174,7 +3203,9 @@ struct gl_framebuffer
/**
* The maximum number of layers in the framebuffer, or 0 if the framebuffer
* is not layered. For cube maps and cube map arrays, each cube face
-* counts as a layer.
+* counts as a layer. As the case for Width, Height a backend driver
+* supporting GL_ARB_framebuffer_no_attachments must use DefaultGeometry
+* in the case that
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
Subject: mesa:Define extension ARB_framebuffer_no_attachments
Space after mesa:
On Thu, May 21, 2015 at 2:30 PM, kevin.rogo...@intel.com wrote:
From: Kevin Rogovin kevin.rogo...@intel.com
Define the infrastructure for the extension GL_ARB_framebuffer_no_attachments:
- extension table
- additions to gl_framebuffer
v1 - v2
Spacing and trailing spaces fixes.
v2 - v3
mtypes.h: Correct comment on _HasAttachments.
Signed-off-by: Kevin Rogovin kevin.rogo...@intel.com
---
src/mesa/main/extensions.c | 1 +
src/mesa/main/fbobject.c| 1 +
src/mesa/main/framebuffer.c | 1 +
src/mesa/main/mtypes.h | 50
-
4 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/src/mesa/main/extensions.c b/src/mesa/main/extensions.c
index c82416a..3256b2c 100644
--- a/src/mesa/main/extensions.c
+++ b/src/mesa/main/extensions.c
@@ -117,6 +117,7 @@ static const struct extension extension_table[] = {
{ GL_ARB_fragment_program,o(ARB_fragment_program),
GLL,2002 },
{ GL_ARB_fragment_program_shadow,
o(ARB_fragment_program_shadow), GLL,2003 },
{ GL_ARB_fragment_shader, o(ARB_fragment_shader),
GL, 2002 },
+ { GL_ARB_framebuffer_no_attachments,
o(ARB_framebuffer_no_attachments), GL, 2012 },
{ GL_ARB_framebuffer_object,
o(ARB_framebuffer_object), GL, 2005 },
{ GL_ARB_framebuffer_sRGB,o(EXT_framebuffer_sRGB),
GL, 1998 },
{ GL_ARB_get_program_binary, o(dummy_true),
GL, 2010 },
diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
index 1859c27..8fea7f8 100644
--- a/src/mesa/main/fbobject.c
+++ b/src/mesa/main/fbobject.c
@@ -957,6 +957,7 @@ _mesa_test_framebuffer_completeness(struct gl_context
*ctx,
fb-Height = 0;
fb-_AllColorBuffersFixedPoint = GL_TRUE;
fb-_HasSNormOrFloatColorBuffer = GL_FALSE;
+ fb-_HasAttachments = GL_TRUE;
/* Start at -2 to more easily loop over all attachment points.
* -2: depth buffer
diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
index 665a5ba..c2cfb92 100644
--- a/src/mesa/main/framebuffer.c
+++ b/src/mesa/main/framebuffer.c
@@ -157,6 +157,7 @@ _mesa_initialize_window_framebuffer(struct gl_framebuffer
*fb,
fb-_Status = GL_FRAMEBUFFER_COMPLETE_EXT;
fb-_AllColorBuffersFixedPoint = !visual-floatMode;
fb-_HasSNormOrFloatColorBuffer = visual-floatMode;
+ fb-_HasAttachments = GL_TRUE;
compute_depth_max(fb);
}
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 8342517..1a37aa6 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -3146,12 +3146,29 @@ struct gl_framebuffer
*/
struct gl_config Visual;
- GLuint Width, Height; /** size of frame buffer in pixels */
+ /**
+* size of frame buffer in pixels,
+* no attachments has these values as 0
What does this mean? That if there are no attachments, these are 0?
Maybe capitalize something. All lower-case in a multiline comment
looks sloppy.
+*/
+ GLuint Width, Height;
+
+ /**
+* In the case that the framebuffer has no attachment (i.e.
+* GL_ARB_framebuffer_no_attachments) then the geometry of
+* the framebuffer is specified by the default values.
+*/
+ struct {
+ GLuint Width, Height, Layers, NumSamples;
+ GLboolean FixedSampleLocations;
+ } DefaultGeometry;
- /** \name Drawing bounds (Intersection of buffer size and scissor box) */
+ /** \name Drawing bounds (Intersection of buffer size and scissor box)
+* The drawing region is given by [_Xmin, _Xmax) x [_Ymin, _Ymax),
+* (inclusive for _Xmin and _Ymin while exclusive for _Xmax and _Ymax)
+*/
/*@{*/
- GLint _Xmin, _Xmax; /** inclusive */
- GLint _Ymin, _Ymax; /** exclusive */
+ GLint _Xmin, _Xmax;
+ GLint _Ymin, _Ymax;
/*@}*/
/** \name Derived Z buffer stuff */
@@ -3164,6 +3181,18 @@ struct gl_framebuffer
/** One of the GL_FRAMEBUFFER_(IN)COMPLETE_* tokens */
GLenum _Status;
+ /** True if one of Attachment has Type != GL_NONE
Whether instead of True if -- I know lots of other places say
True if but it sounds silly.
+* NOTE: the values for Width and Height are set to 0 in
+* case of no attachments, a backend driver supporting
+* GL_ARB_framebuffer_no_attachments must check for the
+* flag _HasAttachments and if GL_FALSE, must then use
+* the values in DefaultGeometry to initialize its
+* viewport, scissor and so on (in particular _Xmin,
+* _Xmax, _Ymin and _Ymax do NOT take into account
+* _HasAttachments being false)
This is terribly
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
Some commit message nits... :)
On 2015-05-21 14:30:48, wrote:
From: Kevin Rogovin kevin.rogo...@intel.com
Define the infrastructure for the extension GL_ARB_framebuffer_no_attachments:
This line is too long. (It will not fit in 80 columns in git log since
git log adds some spaces before each commit message line.)
- extension table
- additions to gl_framebuffer
v1 - v2
Spacing and trailing spaces fixes.
This looks odd to me. I think you only need 'v2:' here. So, either
v2: Spacing and trailing spaces fixes
or,
v2:
* Spacing and trailing spaces fixes
-Jordan
v2 - v3
mtypes.h: Correct comment on _HasAttachments.
Signed-off-by: Kevin Rogovin kevin.rogo...@intel.com
---
src/mesa/main/extensions.c | 1 +
src/mesa/main/fbobject.c| 1 +
src/mesa/main/framebuffer.c | 1 +
src/mesa/main/mtypes.h | 50
-
4 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/src/mesa/main/extensions.c b/src/mesa/main/extensions.c
index c82416a..3256b2c 100644
--- a/src/mesa/main/extensions.c
+++ b/src/mesa/main/extensions.c
@@ -117,6 +117,7 @@ static const struct extension extension_table[] = {
{ GL_ARB_fragment_program,o(ARB_fragment_program),
GLL,2002 },
{ GL_ARB_fragment_program_shadow,
o(ARB_fragment_program_shadow), GLL,2003 },
{ GL_ARB_fragment_shader, o(ARB_fragment_shader),
GL, 2002 },
+ { GL_ARB_framebuffer_no_attachments,
o(ARB_framebuffer_no_attachments), GL, 2012 },
{ GL_ARB_framebuffer_object,
o(ARB_framebuffer_object), GL, 2005 },
{ GL_ARB_framebuffer_sRGB,o(EXT_framebuffer_sRGB),
GL, 1998 },
{ GL_ARB_get_program_binary, o(dummy_true),
GL, 2010 },
diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
index 1859c27..8fea7f8 100644
--- a/src/mesa/main/fbobject.c
+++ b/src/mesa/main/fbobject.c
@@ -957,6 +957,7 @@ _mesa_test_framebuffer_completeness(struct gl_context
*ctx,
fb-Height = 0;
fb-_AllColorBuffersFixedPoint = GL_TRUE;
fb-_HasSNormOrFloatColorBuffer = GL_FALSE;
+ fb-_HasAttachments = GL_TRUE;
/* Start at -2 to more easily loop over all attachment points.
* -2: depth buffer
diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
index 665a5ba..c2cfb92 100644
--- a/src/mesa/main/framebuffer.c
+++ b/src/mesa/main/framebuffer.c
@@ -157,6 +157,7 @@ _mesa_initialize_window_framebuffer(struct gl_framebuffer
*fb,
fb-_Status = GL_FRAMEBUFFER_COMPLETE_EXT;
fb-_AllColorBuffersFixedPoint = !visual-floatMode;
fb-_HasSNormOrFloatColorBuffer = visual-floatMode;
+ fb-_HasAttachments = GL_TRUE;
compute_depth_max(fb);
}
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 8342517..1a37aa6 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -3146,12 +3146,29 @@ struct gl_framebuffer
*/
struct gl_config Visual;
- GLuint Width, Height; /** size of frame buffer in pixels */
+ /**
+* size of frame buffer in pixels,
+* no attachments has these values as 0
+*/
+ GLuint Width, Height;
+
+ /**
+* In the case that the framebuffer has no attachment (i.e.
+* GL_ARB_framebuffer_no_attachments) then the geometry of
+* the framebuffer is specified by the default values.
+*/
+ struct {
+ GLuint Width, Height, Layers, NumSamples;
+ GLboolean FixedSampleLocations;
+ } DefaultGeometry;
- /** \name Drawing bounds (Intersection of buffer size and scissor box) */
+ /** \name Drawing bounds (Intersection of buffer size and scissor box)
+* The drawing region is given by [_Xmin, _Xmax) x [_Ymin, _Ymax),
+* (inclusive for _Xmin and _Ymin while exclusive for _Xmax and _Ymax)
+*/
/*@{*/
- GLint _Xmin, _Xmax; /** inclusive */
- GLint _Ymin, _Ymax; /** exclusive */
+ GLint _Xmin, _Xmax;
+ GLint _Ymin, _Ymax;
/*@}*/
/** \name Derived Z buffer stuff */
@@ -3164,6 +3181,18 @@ struct gl_framebuffer
/** One of the GL_FRAMEBUFFER_(IN)COMPLETE_* tokens */
GLenum _Status;
+ /** True if one of Attachment has Type != GL_NONE
+* NOTE: the values for Width and Height are set to 0 in
+* case of no attachments, a backend driver supporting
+* GL_ARB_framebuffer_no_attachments must check for the
+* flag _HasAttachments and if GL_FALSE, must then use
+* the values in DefaultGeometry to initialize its
+* viewport, scissor and so on (in particular _Xmin,
+* _Xmax, _Ymin and _Ymax do NOT take into account
+* _HasAttachments being false)
+*/
+ GLboolean _HasAttachments;
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
This line is too long. (It will not fit in 80 columns in git log since git log adds some spaces before each commit message line.) What is the accepted maximum length for a line in a commit message? - extension table - additions to gl_framebuffer v1 - v2 Spacing and trailing spaces fixes. This looks odd to me. I think you only need 'v2:' here. So, either I have seen a number of patches with the notation v1 - v2 to list changes between versions. Those patches that I saw using that notation did not have comments about using the format v1-v2. If people want v2: instead of v1-v2, I am fine with it, but was just following what I saw in some patch series. Given the number of nits around (that I seem to hit regularly), it might be beneficial for Mesa to have a short document listing the formatting requirements, of which I have so far collected: 1. 80 column limit in source files 2. Justify comments to 80 columns as well 3. comparison expressions require spaces around both sides of comparison operator 4. successive parenthesis must have spaces between parenthesis 5. git commit messages have limit of N characters per line (the value of N I am asking above). 6. Use whether condition when describing a bool instead of true if condition is true 7. derived values of structs -should- be prefixed with an underscore (this rule is loaded with exceptions in the code base from its evolution) 8. indenting is 3 spaces, except after switch where it is 0 (but after case it is 3). 9. open bracket on same line 10. no white spaces at end of line 11. functions for an extension must check if extension is supported and if not emit an INVALID_OPERATION message instead of relying on function table dispatch to guarantee they are not called. 12. (Guessing here) consecutive empty lines are not allowed 13. If changing a line that violates the nit rules, fix the line too rather than just adding the change I suspect there are more as I listen to the nits, I think it might be beneficial to collect all the formatting nits and write them down to the coding standard thing in Mesa so that others can refer to it. Especially useful for those that work on multiple projects with different coding standards. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
HI, Or 78 columns, to be safe, but there's exceptions, like if you're defining a big static table/array of info. Uggg I don't mind exceptions, but knowing them is key. 4. successive parenthesis must have spaces between parenthesis Example? if (some_func(some_argument)) is bad, but if (some_func(some_argument) ) is good. I am also guessing that a = foo(bar(x)); is bad and must be changed to a = foo(bar(x) ); 6. Use whether condition when describing a bool instead of true if condition is true not sure we care about that. I hit a nit for it in the series, so someone cared. 7. derived values of structs -should- be prefixed with an underscore (this rule is loaded with exceptions in the code base from its evolution) 8. indenting is 3 spaces, except after switch where it is 0 (but after case it is 3). 9. open bracket on same line The 'indent' command in the docs should cover that. 11. functions for an extension must check if extension is supported and if not emit an INVALID_OPERATION message instead of relying on function table dispatch to guarantee they are not called. Not sure about that, but that's not a coding style standard. Perhaps coding standard is not the right word, but it is a requirement (atleast it seems that way) and is a trivial requirement to satisfy. 12. (Guessing here) consecutive empty lines are not allowed Generally true, except between functions. Ugg... I hit a nit from an extra space between functions. 13. If changing a line that violates the nit rules, fix the line too rather than just adding the change Yeah, probably. Feel free to submit a patch against docs/devinfo.html with this info. :) I do not mind submitting the patch, but I want to know what the rules are; preferably explicitly written rather than inferred (by me). Especially since I seem to hit nits like no tomorrow even when trying not to :) -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
On 05/21/2015 05:05 PM, Rogovin, Kevin wrote: This line is too long. (It will not fit in 80 columns in git log since git log adds some spaces before each commit message line.) What is the accepted maximum length for a line in a commit message? Probably 75 chars since there's 4 spaces of indenting and we try to make things look nice for 80-column terminals and editors. - extension table - additions to gl_framebuffer v1 - v2 Spacing and trailing spaces fixes. This looks odd to me. I think you only need 'v2:' here. So, either I have seen a number of patches with the notation v1 - v2 to list changes between versions. Those patches that I saw using that notation did not have comments about using the format v1-v2. If people want v2: instead of v1-v2, I am fine with it, but was just following what I saw in some patch series. Given the number of nits around (that I seem to hit regularly), it might be beneficial for Mesa to have a short document listing the formatting requirements, of which I have so far collected: 1. 80 column limit in source files 2. Justify comments to 80 columns as well Or 78 columns, to be safe, but there's exceptions, like if you're defining a big static table/array of info. 3. comparison expressions require spaces around both sides of comparison operator Generally, a space on both sides of an operator like +, *, /, , =, etc. 4. successive parenthesis must have spaces between parenthesis Example? 5. git commit messages have limit of N characters per line (the value of N I am asking above). 75. 6. Use whether condition when describing a bool instead of true if condition is true not sure we care about that. 7. derived values of structs -should- be prefixed with an underscore (this rule is loaded with exceptions in the code base from its evolution) 8. indenting is 3 spaces, except after switch where it is 0 (but after case it is 3). 9. open bracket on same line The 'indent' command in the docs should cover that. 10. no white spaces at end of line 11. functions for an extension must check if extension is supported and if not emit an INVALID_OPERATION message instead of relying on function table dispatch to guarantee they are not called. Not sure about that, but that's not a coding style standard. 12. (Guessing here) consecutive empty lines are not allowed Generally true, except between functions. 13. If changing a line that violates the nit rules, fix the line too rather than just adding the change Yeah, probably. I suspect there are more as I listen to the nits, I think it might be beneficial to collect all the formatting nits and write them down to the coding standard thing in Mesa so that others can refer to it. Especially useful for those that work on multiple projects with different coding standards. Feel free to submit a patch against docs/devinfo.html with this info. :) -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
On Thu, May 21, 2015 at 5:05 PM, Rogovin, Kevin kevin.rogo...@intel.com wrote: This line is too long. (It will not fit in 80 columns in git log since git log adds some spaces before each commit message line.) What is the accepted maximum length for a line in a commit message? Gentoo's default vim configuration line wraps the commit message at 72 (I think because 72 + an 8-space tab in git log fits in an 80 column window). It also changes the highlighting of the commit title itself after 50 characters. I think both of those are reasonable, although staying in 50 characters for the title is sometimes hard. - extension table - additions to gl_framebuffer v1 - v2 Spacing and trailing spaces fixes. This looks odd to me. I think you only need 'v2:' here. So, either I have seen a number of patches with the notation v1 - v2 to list changes between versions. Those patches that I saw using that notation did not have comments about using the format v1-v2. If people want v2: instead of v1-v2, I am fine with it, but was just following what I saw in some patch series. Given the number of nits around (that I seem to hit regularly), it might be beneficial for Mesa to have a short document listing the formatting requirements, of which I have so far collected: 1. 80 column limit in source files 2. Justify comments to 80 columns as well 3. comparison expressions require spaces around both sides of comparison operator 4. successive parenthesis must have spaces between parenthesis 5. git commit messages have limit of N characters per line (the value of N I am asking above). 6. Use whether condition when describing a bool instead of true if condition is true 7. derived values of structs -should- be prefixed with an underscore (this rule is loaded with exceptions in the code base from its evolution) 8. indenting is 3 spaces, except after switch where it is 0 (but after case it is 3). 9. open bracket on same line 10. no white spaces at end of line 11. functions for an extension must check if extension is supported and if not emit an INVALID_OPERATION message instead of relying on function table dispatch to guarantee they are not called. 12. (Guessing here) consecutive empty lines are not allowed 13. If changing a line that violates the nit rules, fix the line too rather than just adding the change I suspect there are more as I listen to the nits, I think it might be beneficial to collect all the formatting nits and write them down to the coding standard thing in Mesa so that others can refer to it. Especially useful for those that work on multiple projects with different coding standards. I suppose it could be useful, but I think we've been mostly successful at just expecting people to recognize when what they're writing doesn't look like the code around it. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
On 05/21/2015 05:26 PM, Rogovin, Kevin wrote: HI, Or 78 columns, to be safe, but there's exceptions, like if you're defining a big static table/array of info. Uggg I don't mind exceptions, but knowing them is key. 4. successive parenthesis must have spaces between parenthesis Example? if (some_func(some_argument)) is bad, but if (some_func(some_argument) ) is good. I am also guessing that a = foo(bar(x)); is bad and must be changed to a = foo(bar(x) ); No, I don't think that's our intention. a = foo(bar(x)); would be my choice. Seems perfectly readable. I haven't been following this thread so perhaps I've missed someone else's suggestion. 6. Use whether condition when describing a bool instead of true if condition is true not sure we care about that. I hit a nit for it in the series, so someone cared. Well, we want our comments to be clear, concise and helpful and sometimes the best language is a personal preference. Hard to be specific there. 7. derived values of structs -should- be prefixed with an underscore (this rule is loaded with exceptions in the code base from its evolution) 8. indenting is 3 spaces, except after switch where it is 0 (but after case it is 3). 9. open bracket on same line The 'indent' command in the docs should cover that. 11. functions for an extension must check if extension is supported and if not emit an INVALID_OPERATION message instead of relying on function table dispatch to guarantee they are not called. Not sure about that, but that's not a coding style standard. Perhaps coding standard is not the right word, but it is a requirement (atleast it seems that way) and is a trivial requirement to satisfy. 12. (Guessing here) consecutive empty lines are not allowed Generally true, except between functions. Ugg... I hit a nit from an extra space between functions. 13. If changing a line that violates the nit rules, fix the line too rather than just adding the change Yeah, probably. Feel free to submit a patch against docs/devinfo.html with this info. :) I do not mind submitting the patch, but I want to know what the rules are; preferably explicitly written rather than inferred (by me). Especially since I seem to hit nits like no tomorrow even when trying not to :) I can understand your frustration. Going around and around with tiny changes isn't fun. But I think we're all interested in getting things to look right the first time, rather than having to clean it up later. Thanks for your patience and persistence. -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
I suppose it could be useful, but I think we've been mostly successful at just expecting people to recognize when what they're writing doesn't look like the code around it. This is my point. Older code had different style/expectations than newer code. For this patch series, I have hit a number of nits and quasi-nits that are ambiguous: 0. In v1 to v2, avoid GL types atleast in i965 although much of the code uses that (the explanation given was to move away from GL types). Does this apply to Mesa core as well? 1. the casting bit in the i965 patch to use the _mesa_geomety_ functions. These are not quite nits, but in some ways not a big deal. I hit these because there were ints there before. In this regard doing what was there before was ungood (atleast for this review). 2. the line between function thing. In truth I just missed that extra line for the added to framebuffer.h (and I should not have), but there are places in the code that there are multiple empty lines between function definitions. I do not mind saying no extra empty lines, but not knowing the rules and attempting to infer them from the code, I seem to hit too many nits. 3. Even on the subject of git commit, I am seeing different answers: 75, but try 50 usually, but understandable if cannot do it. Utterly ambiguous. 4. on the subject of line length I have this so far: usually 78, but 80 sometimes is ok. Does ok, for example, include making a large-ish comment block more justified? 5. Consecutive empty lines is not ok, except in function definitions, but then only sometimes. I am guessing sometimes is for grouping function definitions, but plenty of files follow different conventions (hence what Brian Paul said). Given that nits just add traffic (and I want to minimize that silliness) I think it would be wise to set down some precise rules so there is no judgement calls required for something as silly as formatting nits. Ideally, we would have a lint script that would do it for us :) I see that reviews usually first hit nits, then content of patches. That is fine, but I'd rather know all the rules rather than hitting the nits at all. Again, I really have no preference since someone is paying me to do this, but knowing the exact rules would be more efficient. Inferring rules is quite error prone IMO (indeed, at one point, the GLSL pre-processor appeared to be written in a different style). -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
On Thu, May 21, 2015 at 8:05 PM, Rogovin, Kevin kevin.rogo...@intel.com wrote: This line is too long. (It will not fit in 80 columns in git log since git log adds some spaces before each commit message line.) What is the accepted maximum length for a line in a commit message? - extension table - additions to gl_framebuffer v1 - v2 Spacing and trailing spaces fixes. This looks odd to me. I think you only need 'v2:' here. So, either I have seen a number of patches with the notation v1 - v2 to list changes between versions. Those patches that I saw using that notation did not have comments about using the format v1-v2. If people want v2: instead of v1-v2, I am fine with it, but was just following what I saw in some patch series. Given the number of nits around (that I seem to hit regularly), it might be beneficial for Mesa to have a short document listing the formatting requirements, of which I have so far collected: 1. 80 column limit in source files 2. Justify comments to 80 columns as well 3. comparison expressions require spaces around both sides of comparison operator 4. successive parenthesis must have spaces between parenthesis 5. git commit messages have limit of N characters per line (the value of N I am asking above). 6. Use whether condition when describing a bool instead of true if condition is true 7. derived values of structs -should- be prefixed with an underscore (this rule is loaded with exceptions in the code base from its evolution) 8. indenting is 3 spaces, except after switch where it is 0 (but after case it is 3). 9. open bracket on same line 10. no white spaces at end of line 11. functions for an extension must check if extension is supported and if not emit an INVALID_OPERATION message instead of relying on function table dispatch to guarantee they are not called. 12. (Guessing here) consecutive empty lines are not allowed 13. If changing a line that violates the nit rules, fix the line too rather than just adding the change I suspect there are more as I listen to the nits, I think it might be beneficial to collect all the formatting nits and write them down to the coding standard thing in Mesa so that others can refer to it. Especially useful for those that work on multiple projects with different coding standards. FWIW, the kernel standards for commit messages are at: https://www.kernel.org/doc/Documentation/SubmittingPatches Most of those rules apply to Mesa too. It says the body should be wrapped to 75 chars (although I've been using 72 like Matt said). -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
On Thu, May 21, 2015 at 5:26 PM, Rogovin, Kevin kevin.rogo...@intel.com wrote: HI, Or 78 columns, to be safe, but there's exceptions, like if you're defining a big static table/array of info. Uggg I don't mind exceptions, but knowing them is key. 4. successive parenthesis must have spaces between parenthesis Example? if (some_func(some_argument)) is bad, but if (some_func(some_argument) ) is good. I am also guessing that a = foo(bar(x)); is bad and must be changed to a = foo(bar(x) ); That's not a style I've ever seen. Some old Mesa code always puts spaces around the condition in an if statement, like if ( cond ), but there's nothing about just an extra space at the end. 6. Use whether condition when describing a bool instead of true if condition is true not sure we care about that. I hit a nit for it in the series, so someone cared. 7. derived values of structs -should- be prefixed with an underscore (this rule is loaded with exceptions in the code base from its evolution) 8. indenting is 3 spaces, except after switch where it is 0 (but after case it is 3). 9. open bracket on same line The 'indent' command in the docs should cover that. 11. functions for an extension must check if extension is supported and if not emit an INVALID_OPERATION message instead of relying on function table dispatch to guarantee they are not called. Not sure about that, but that's not a coding style standard. Perhaps coding standard is not the right word, but it is a requirement (atleast it seems that way) and is a trivial requirement to satisfy. 12. (Guessing here) consecutive empty lines are not allowed Generally true, except between functions. Ugg... I hit a nit from an extra space between functions. You hit a nit because you were inconsistent. Look at your patch. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
FWIW, the kernel standards for commit messages are at: https://www.kernel.org/doc/Documentation/SubmittingPatches Most of those rules apply to Mesa too. It says the body should be wrapped to 75 chars (although I've been using 72 like Matt said). This is my point: use most rules, but not all.. and I've been more conservative than X but I did not need to be. What I am seeing is that there is, in some collective form, a set of consistent rules (in the form of ranges) that are strongly enforced and yet not written down. Let's write them all down here and now, put them in some file for others to read and to refer to. Maybe even con someone to write a lint-like script for those rules. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
On Thu, May 21, 2015 at 8:40 PM, Rogovin, Kevin kevin.rogo...@intel.com wrote: I suppose it could be useful, but I think we've been mostly successful at just expecting people to recognize when what they're writing doesn't look like the code around it. This is my point. Older code had different style/expectations than newer code. For this patch series, I have hit a number of nits and quasi-nits that are ambiguous: 0. In v1 to v2, avoid GL types atleast in i965 although much of the code uses that (the explanation given was to move away from GL types). Does this apply to Mesa core as well? 1. the casting bit in the i965 patch to use the _mesa_geomety_ functions. These are not quite nits, but in some ways not a big deal. I hit these because there were ints there before. In this regard doing what was there before was ungood (atleast for this review). 2. the line between function thing. In truth I just missed that extra line for the added to framebuffer.h (and I should not have), but there are places in the code that there are multiple empty lines between function definitions. I do not mind saying no extra empty lines, but not knowing the rules and attempting to infer them from the code, I seem to hit too many nits. 3. Even on the subject of git commit, I am seeing different answers: 75, but try 50 usually, but understandable if cannot do it. Utterly ambiguous. No, that's because you misread it. The rules for commit messages (and usually, for everything else as well) have a purpose, and that's why they might seem ambiguous to you: we're adults here, so we don't need to have a huge debate on 75 vs. 72 chars for commit message bodies, because we know that both satisfy the *purpose* of being able to do 'git log' on an 80-char terminal without ugly line-wrapping due to 'git log' indenting your message an extra 4 spaces. Sometimes we get too wrapped up in sweating the details, but in the end it's just about consistency and making things easier for people. You're also missing the difference between the subject line, which is the thing people will skim over when looking for something and therefore should be as short as possible (ideally under 50 chars), and the body (the part after the empty line), which is what people already looking at your commit will read to fully understand what it's changing. The body can be as long as it needs to be, as long as it's concise and to the point, and it should be wrapped to at most 75 chars. 4. on the subject of line length I have this so far: usually 78, but 80 sometimes is ok. Does ok, for example, include making a large-ish comment block more justified? 5. Consecutive empty lines is not ok, except in function definitions, but then only sometimes. I am guessing sometimes is for grouping function definitions, but plenty of files follow different conventions (hence what Brian Paul said). Given that nits just add traffic (and I want to minimize that silliness) I think it would be wise to set down some precise rules so there is no judgement calls required for something as silly as formatting nits. Ideally, we would have a lint script that would do it for us :) I see that reviews usually first hit nits, then content of patches. That is fine, but I'd rather know all the rules rather than hitting the nits at all. Again, I really have no preference since someone is paying me to do this, but knowing the exact rules would be more efficient. Inferring rules is quite error prone IMO (indeed, at one point, the GLSL pre-processor appeared to be written in a different style). -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments
On Thu, May 21, 2015 at 8:51 PM, Rogovin, Kevin kevin.rogo...@intel.com wrote: FWIW, the kernel standards for commit messages are at: https://www.kernel.org/doc/Documentation/SubmittingPatches Most of those rules apply to Mesa too. It says the body should be wrapped to 75 chars (although I've been using 72 like Matt said). This is my point: use most rules, but not all.. and I've been more conservative than X but I did not need to be. What I am seeing is that there is, in some collective form, a set of consistent rules (in the form of ranges) that are strongly enforced and yet not written down. Let's write them all down here and now, put them in some file for others to read and to refer to. Maybe even con someone to write a lint-like script for those rules. -Kevin I said use most rules, but all because I'm expecting you to use your common sense to see what rules apply and which might need to be modified to apply to Mesa. For example, as you've probably noticed, Mesa doesn't have any formal maintainers like Linux does, so the process for getting your patch into the tree goes from get a sign-off from the maintainer and they'll merge it into their tree to get a reviewed-by from someone well-known and familiar with the code you're modifying, and they'll commit it if you don't have commit access. Usually figuring out who that is pretty straightforward (hint: git blame), but if not you can ask. We don't have a style guide or the equivalent of checkpatch.pl for the same reason we don't have formal maintainers: the project isn't large enough, and doesn't have enough infrastructure, for it. There haven't been enough people like you that have to be hand-fed every detail to justify the work; instead, we just note problems when they occur and rely on the patch author to fix them. If there's anyone to be conned to do that work, it's going to be you. You've certainly been given enough information by now to be able to do so. Finally, I'll quote a section of the file I linked to: Be sure to tell the reviewers what changes you are making and to thank them for their time. Code review is a tiring and time-consuming process, and reviewers sometimes get grumpy. Even in that case, though, respond politely and address the problems they have pointed out. Everyone makes mistakes. Heck, I just sent out a series with some minor style nits that Matt pointed out. When that happens, you fix the problem, try to remember it for the future, and then move on. Starting a 13 (now 14) email thread where you do nothing but complain about it is a great way to not get your patches merged, and that would be a good thing to remember in case you actually care about getting them merged; but if you don't, then why did you send them and waste our time in the first place? Connor ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
