Parse http://www.opengl.org/registry/#specfiles to generate all constants.

One advise : don't loose your time to review GLConsts.h, this is a generated file.

Comment on attachment 784507 [details] [diff] [review] patch revision 1 Review of attachment 784507 [details] [diff] [review]: ----------------------------------------------------------------- As we discussed, this needs to be split into multiple patches; the first one, that should be uncontroversial, would just parse the XML and generate GLDefs as-is from it (with #define LOCAL_GL_...). Then we could talk separately about replacing #define's by constants, and about changing LOCAL_GL_ to something else.

I don't want to bikeshed, but there's tons of code out there that already does this. Why reinvent the wheel, instead of pulling in the generated include file from glew, regal, etc? I guess at least it's parsing xml instead of parsing the actual spec files themselves, so the amount of code isn't too bad, but we still have to maintain it.

This patch parse the official registry xmls to #define LOCAL_GL_FOO_BAR 0xABCD. We have still some def in GLDefs.h because they are not specified. We should fix that in another bug.

Comment on attachment 785324 [details] [diff] [review] patch revision 5 Review of attachment 785324 [details] [diff] [review]: ----------------------------------------------------------------- r=me with questions / comments ::: gfx/gl/GLConsts.h @@ +5,5 @@ > +#if !defined(GLCONSTS_H_) > +#define GLCONSTS_H_ > + > +/** > + * DO NOT INTEND TO MODIFY THIS FILE. ALL CHANGES MIGHT BE LOST. GENERATED FILE, DO NOT MODIFY ::: gfx/gl/GLDefs.h @@ -78,5 @@ > -# define GLAPI > -# endif > -#endif > - > -#define LOCAL_GL_VERSION_1_1 1 Wait, why not #include GLConsts.h ? ::: gfx/gl/GLParseRegistryXML.py @@ +13,5 @@ > +# Step 2: > +# Execute this script > +# > +# Step 3: > +# Do not add the download xml in the patch downloaded? @@ +49,5 @@ > + f.write('\n') > + f.write('#endif // ' + fileDefine + '_H_\n') > + > +def getGLType(type): > + lots of trailing \w in this file.

Comment on attachment 785324 [details] [diff] [review] patch revision 5 Review of attachment 785324 [details] [diff] [review]: ----------------------------------------------------------------- The pythons script desperately needs comments detailing when it's doing what, and why it does what it does. Also, I think our best bet for transitioning between GL::kFOO and LOCAL_GL_FOO is: 1) Generate GLConsts.h in the form GL::kFOO 2) Switch all our uses of LOCAL_GL_ to GL::k 3) Remove GLDefs.h ::: gfx/gl/GLConsts.h @@ +15,5 @@ > + > +#include "GLTypes.h" > + > + > +//--------------------------------------------------------------------------- GL IMO: //---------... // GL ::: gfx/gl/GLParseRegistryXML.py @@ +7,5 @@ > +# > +# Step 1: > +# Download the last gl.xml, egl.xml, glx.xml and wgl.xml from > +# http://www.opengl.org/registry/#specfiles and put it into the gl/gfx/ > +# directory I don't like that this recommends leaving cruft in someone's gfx/gl dir. Also "gfx/gl" not "gl/gfx". @@ +31,5 @@ > +def fileGenerateHeader(f,fileDefine): > + f.write('/* This Source Code Form is subject to the terms of the Mozilla Public\n') > + f.write(' * License, v. 2.0. If a copy of the MPL was not distributed with this\n') > + f.write(' * file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n') > + f.write('\n') The nicer way to do this is generally: f.write("\n".join([ 'line1', 'line2' ])) @@ +76,5 @@ > + def access(self): > + return accessComposer(self.lib,self.name) > + > + > +class GLDataBase: "database" is one word @@ +89,5 @@ > + self.vendors['ARB'] = 1 > + self.vendors['EXT'] = 1 > + > + for lib in GLDataBase.LIBS: > + self.libs[lib] = 1 Why are we setting these vendors and libs to one? @@ +93,5 @@ > + self.libs[lib] = 1 > + > + > + def loadXml(self,path): > + Why a blank line after a `def:`? @@ +94,5 @@ > + > + > + def loadXml(self,path): > + > + tree = xml.etree.ElementTree.parse(getScriptDir()+path) Spaces around operators. (` + `) Also, this should handle failure cases gracefully: What if someone forgets a file? @@ +98,5 @@ > + tree = xml.etree.ElementTree.parse(getScriptDir()+path) > + root = tree.getroot() > + > + for enums in root.iter('enums'): > + Why have a blank line after `for:`? @@ +101,5 @@ > + for enums in root.iter('enums'): > + > + vendor = enums.get('vendor') > + if not vendor: > + vendor = 'ARB' Why do we set it to ARB? @@ +104,5 @@ > + if not vendor: > + vendor = 'ARB' > + > + if vendor not in self.vendors: > + self.vendors[vendor] = 1 What does `1` mean here? @@ +132,5 @@ > + > + > + def getConstValue(self,const): > + > + searchName = 'GL_' + const.name Shouldn't we use const.lib instead of always 'GL_'? @@ +136,5 @@ > + searchName = 'GL_' + const.name > + > + nameSuffix = searchName.split('_')[-1] > + if nameSuffix in self.vendors: > + searchName = searchName[0:len(searchName)-(len(nameSuffix)+1)] Spaces around operators. Also, iirc, this should be `searchName[:-len(nameSuffix)]` @@ +158,5 @@ > + > + f = open(getScriptDir()+path,'w') > + > + fileDefine = 'GLCONSTS' > + fileGenerateHeader(f,fileDefine) Spaces after commas.

Arg, can someone let me know why we're moving from LOCAL_GL_FOO to GL::kFOO? I understand that LOCAL_ is ugly (the code started out as webgl-only!), but GL::k is just as ugly. If we hate the LOCAL_, then replace it with MOZ_.. using "GL::k" as the prefix doesn't really buy much other, does it?

The choice of prefix seems controversial; Jeff M expressed similar concerns. Perhaps the least controversial (aside from keeping LOCAL_GL_) would be to just find a way to make GL_ work. I know we had conflicts and had a reason to introduce LOCAL_GL_, but somehow Chromium manages to have GL_. Maybe some custom #undef / #ifdef'ing would do.

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #7) > Arg, can someone let me know why we're moving from LOCAL_GL_FOO to GL::kFOO? > I understand that LOCAL_ is ugly (the code started out as webgl-only!), but > GL::k is just as ugly. If we hate the LOCAL_, then replace it with MOZ_.. > using "GL::k" as the prefix doesn't really buy much other, does it? I don't think MOZ_ is better enough than LOCAL_ to incentivize a change. What about keeping LOCAL_GL_FOO around, but transitioning towards GL::FOO for compatible symbols. (most of them) GL::FRAMEBUFFER, but only LOCAL_GL_TRUE, no GL::TRUE. `GL::` would then become a sort of syntactic sugar for `LOCAL_GL_`. If history didn't hold us back, I would even suggest GL::FramebufferBinding and GL::ColorAttachment0.

After chatting with Jeff M, I came to agree with him that GL_ was actually better than GL:: because: 1) it is the thing that people are most used to 2) it can be applied unconditionally to all constants, which is nicer than having special cases. 3) a typical strong point of C++ namespaces is 'using' (or being inside the namespace); but in this case, this is rather counter-productive as stripping the prefix makes the identifier less explicit. For example, everyone knows what GL_RGBA is, but RGBA could be from one out of many sets of constants that we have.

GL::Foo would be a win, but I'm with bjacob -- GL_ is definitely better. Let's figure out what's hurting us there? I suspect we can get there with some judicious #undef'ing. iirc, the biggest problem came from OSX.

(In reply to Benoit Jacob [:bjacob] from comment #5) > Comment on attachment 785324 [details] [diff] [review] > patch revision 5 > > Review of attachment 785324 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with questions / comments > > ::: gfx/gl/GLConsts.h > @@ +5,5 @@ > > +#if !defined(GLCONSTS_H_) > > +#define GLCONSTS_H_ > > + > > +/** > > + * DO NOT INTEND TO MODIFY THIS FILE. ALL CHANGES MIGHT BE LOST. > > GENERATED FILE, DO NOT MODIFY Fixed. > > ::: gfx/gl/GLDefs.h > @@ -78,5 @@ > > -# define GLAPI > > -# endif > > -#endif > > - > > -#define LOCAL_GL_VERSION_1_1 1 > > Wait, why not #include GLConsts.h ? It is, the file is just not reviewable... > > ::: gfx/gl/GLParseRegistryXML.py > @@ +13,5 @@ > > +# Step 2: > > +# Execute this script > > +# > > +# Step 3: > > +# Do not add the download xml in the patch > > downloaded? Fixed. > > @@ +49,5 @@ > > + f.write('\n') > > + f.write('#endif // ' + fileDefine + '_H_\n') > > + > > +def getGLType(type): > > + > > lots of trailing \w in this file. Oups...

(In reply to Jeff Gilbert [:jgilbert] from comment #6) > Comment on attachment 785324 [details] [diff] [review] > patch revision 5 > > Review of attachment 785324 [details] [diff] [review]: > ----------------------------------------------------------------- > > The pythons script desperately needs comments detailing when it's doing > what, and why it does what it does. > > Also, I think our best bet for transitioning between GL::kFOO and > LOCAL_GL_FOO is: > 1) Generate GLConsts.h in the form GL::kFOO > 2) Switch all our uses of LOCAL_GL_ to GL::k > 3) Remove GLDefs.h > > ::: gfx/gl/GLConsts.h > @@ +15,5 @@ > > + > > +#include "GLTypes.h" > > + > > + > > +//--------------------------------------------------------------------------- GL > > IMO: > //---------... > // GL > > ::: gfx/gl/GLParseRegistryXML.py > @@ +7,5 @@ > > +# > > +# Step 1: > > +# Download the last gl.xml, egl.xml, glx.xml and wgl.xml from > > +# http://www.opengl.org/registry/#specfiles and put it into the gl/gfx/ > > +# directory > > I don't like that this recommends leaving cruft in someone's gfx/gl dir. Would you rather like a parameter for script to indicate the directory where the xml are? > > Also "gfx/gl" not "gl/gfx". Fixed. > > @@ +31,5 @@ > > +def fileGenerateHeader(f,fileDefine): > > + f.write('/* This Source Code Form is subject to the terms of the Mozilla Public\n') > > + f.write(' * License, v. 2.0. If a copy of the MPL was not distributed with this\n') > > + f.write(' * file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n') > > + f.write('\n') > > The nicer way to do this is generally: > f.write("\n".join([ > 'line1', > 'line2' > ])) I don't like what your are proposing because it would generate f.write('line1\nline2'); instead of f.write('line1\nline2\n'); > > @@ +76,5 @@ > > + def access(self): > > + return accessComposer(self.lib,self.name) > > + > > + > > +class GLDataBase: > > "database" is one word Oups... > > @@ +89,5 @@ > > + self.vendors['ARB'] = 1 > > + self.vendors['EXT'] = 1 > > + > > + for lib in GLDataBase.LIBS: > > + self.libs[lib] = 1 > > Why are we setting these vendors and libs to one? I'm using a map to have a logarithmic complexity for checking if a word is a vendor. Therefore the key is important, and the value doesn't matter. That why I set to 1 for every one. [0] > > @@ +93,5 @@ > > + self.libs[lib] = 1 > > + > > + > > + def loadXml(self,path): > > + > > Why a blank line after a `def:`? Why not? Let's keep a readable script instead of a highly concentrated script. [1] > > @@ +94,5 @@ > > + > > + > > + def loadXml(self,path): > > + > > + tree = xml.etree.ElementTree.parse(getScriptDir()+path) > > Spaces around operators. (` + `) Fixed. > Also, this should handle failure cases gracefully: What if someone forgets a > file? The python fail on xml.etree.ElementTree.parse(getScriptDir()+path), and GLConsts.h is not changed. > > @@ +98,5 @@ > > + tree = xml.etree.ElementTree.parse(getScriptDir()+path) > > + root = tree.getroot() > > + > > + for enums in root.iter('enums'): > > + > > Why have a blank line after `for:`? [1] > > @@ +101,5 @@ > > + for enums in root.iter('enums'): > > + > > + vendor = enums.get('vendor') > > + if not vendor: > > + vendor = 'ARB' > > Why do we set it to ARB? OpenGL 1.0 features don't have official vendors, for historical reasons. So because this is in a 'Core profile' I fake it as ARB vendor. > > @@ +104,5 @@ > > + if not vendor: > > + vendor = 'ARB' > > + > > + if vendor not in self.vendors: > > + self.vendors[vendor] = 1 > > What does `1` mean here? [0] > > @@ +132,5 @@ > > + > > + > > + def getConstValue(self,const): > > + > > + searchName = 'GL_' + const.name > > Shouldn't we use const.lib instead of always 'GL_'? Librairies are rendered in GLConsts.h by the order indicated in GLDatabase.LIBS. So this function, just try to find a equivalent consts in the multiplatform library 'GL', even for other libraries. For example, that let us have: #define LOCAL_EGL_FALSE LOCAL_GL_FALSE instead of: #define LOCAL_EGL_FALSE 0 Which is beater for readability because we clearly see that LOCAL_EGL_FALSE and LOCAL_GL_FALSE are compatible. > > @@ +136,5 @@ > > + searchName = 'GL_' + const.name > > + > > + nameSuffix = searchName.split('_')[-1] > > + if nameSuffix in self.vendors: > > + searchName = searchName[0:len(searchName)-(len(nameSuffix)+1)] > > Spaces around operators. Fixed. > Also, iirc, this should be `searchName[:-len(nameSuffix)]` > > @@ +158,5 @@ > > + > > + f = open(getScriptDir()+path,'w') > > + > > + fileDefine = 'GLCONSTS' > > + fileGenerateHeader(f,fileDefine) > > Spaces after commas. Oups...

Yeah, so it has been approved by a major part that we would like to generate all LOCAL_GL_ from the xml first, an then decide next Monday on the rendering meeting, the new look of GL constants.

(In reply to Guillaume Abadie from comment #13) > (In reply to Jeff Gilbert [:jgilbert] from comment #6) > > ::: gfx/gl/GLConsts.h > > @@ +15,5 @@ > > > + > > > +#include "GLTypes.h" > > > + > > > + > > > +//--------------------------------------------------------------------------- GL > > > > IMO: > > //---------... > > // GL > > Thoughts? > > ::: gfx/gl/GLParseRegistryXML.py > > @@ +7,5 @@ > > > +# > > > +# Step 1: > > > +# Download the last gl.xml, egl.xml, glx.xml and wgl.xml from > > > +# http://www.opengl.org/registry/#specfiles and put it into the gl/gfx/ > > > +# directory > > > > I don't like that this recommends leaving cruft in someone's gfx/gl dir. > Would you rather like a parameter for script to indicate the directory where > the xml are? Yeah, that would work. Or just instruct users to copy this file to a new folder to use, and then copy the resulting file back out. > > > > @@ +31,5 @@ > > > +def fileGenerateHeader(f,fileDefine): > > > + f.write('/* This Source Code Form is subject to the terms of the Mozilla Public\n') > > > + f.write(' * License, v. 2.0. If a copy of the MPL was not distributed with this\n') > > > + f.write(' * file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n') > > > + f.write('\n') > > > > The nicer way to do this is generally: > > f.write("\n".join([ > > 'line1', > > 'line2' > > ])) > I don't like what your are proposing because it would generate > f.write('line1\nline2'); instead of f.write('line1\nline2\n'); And the usual response is: f.write('\n'.join([ 'line1', 'line2', '' ])) Alternatively, add a helper function: def WriteLn(line): f.write(line + '\n') WriteLn('\n'.join... > > > > @@ +89,5 @@ > > > + self.vendors['ARB'] = 1 > > > + self.vendors['EXT'] = 1 > > > + > > > + for lib in GLDataBase.LIBS: > > > + self.libs[lib] = 1 > > > > Why are we setting these vendors and libs to one? > I'm using a map to have a logarithmic complexity for checking if a word is a > vendor. Therefore the key is important, and the value doesn't matter. That > why I set to 1 for every one. [0] A map without values associated with keys is just a set, so use a set. :) > > > > @@ +93,5 @@ > > > + self.libs[lib] = 1 > > > + > > > + > > > + def loadXml(self,path): > > > + > > > > Why a blank line after a `def:`? > Why not? Let's keep a readable script instead of a highly concentrated > script. [1] This makes it less readable, in my opinion, and goes against the style we use for c++. Our styles for python and c++ shouldn't be wildly different, and also, other short functions in this file don't follow this style. > > > > @@ +94,5 @@ > > > + > > > + > > > + def loadXml(self,path): > > > + > > > + tree = xml.etree.ElementTree.parse(getScriptDir()+path) > > > > Spaces around operators. (` + `) > Fixed. > > Also, this should handle failure cases gracefully: What if someone forgets a > > file? > The python fail on xml.etree.ElementTree.parse(getScriptDir()+path), and > GLConsts.h is not changed. Does the error it give make it obvious what's wrong? Generally, catch the exception, and give us a readable 'Error: source xml \'' + getScriptDir() + path + '\' not found.'. > > > > @@ +98,5 @@ > > > + tree = xml.etree.ElementTree.parse(getScriptDir()+path) > > > + root = tree.getroot() > > > + > > > + for enums in root.iter('enums'): > > > + > > > > Why have a blank line after `for:`? > [1] > > > > @@ +101,5 @@ > > > + for enums in root.iter('enums'): > > > + > > > + vendor = enums.get('vendor') > > > + if not vendor: > > > + vendor = 'ARB' > > > > Why do we set it to ARB? > OpenGL 1.0 features don't have official vendors, for historical reasons. So > because this is in a 'Core profile' I fake it as ARB vendor. Why? > > > > @@ +104,5 @@ > > > + if not vendor: > > > + vendor = 'ARB' > > > + > > > + if vendor not in self.vendors: > > > + self.vendors[vendor] = 1 > > > > What does `1` mean here? > [0] Same here. > > > > @@ +132,5 @@ > > > + > > > + > > > + def getConstValue(self,const): > > > + > > > + searchName = 'GL_' + const.name > > > > Shouldn't we use const.lib instead of always 'GL_'? > Librairies are rendered in GLConsts.h by the order indicated in > GLDatabase.LIBS. So this function, just try to find a equivalent consts in > the multiplatform library 'GL', even for other libraries. For example, that > let us have: > #define LOCAL_EGL_FALSE LOCAL_GL_FALSE > instead of: > #define LOCAL_EGL_FALSE 0 > Which is beater for readability because we clearly see that LOCAL_EGL_FALSE > and LOCAL_GL_FALSE are compatible. I don't know as I like this. Different libraries deliberately have different namespaces. Also, s/Xml/XML/.

(In reply to Jeff Gilbert [:jgilbert] from comment #15) > (In reply to Guillaume Abadie from comment #13) > > (In reply to Jeff Gilbert [:jgilbert] from comment #6) > > > ::: gfx/gl/GLConsts.h > > > @@ +15,5 @@ > > > > + > > > > +#include "GLTypes.h" > > > > + > > > > + > > > > +//--------------------------------------------------------------------------- GL > > > > > > IMO: > > > //---------... > > > // GL > > > > Thoughts? Fixed. > > > > ::: gfx/gl/GLParseRegistryXML.py > > > @@ +7,5 @@ > > > > +# > > > > +# Step 1: > > > > +# Download the last gl.xml, egl.xml, glx.xml and wgl.xml from > > > > +# http://www.opengl.org/registry/#specfiles and put it into the gl/gfx/ > > > > +# directory > > > > > > I don't like that this recommends leaving cruft in someone's gfx/gl dir. > > Would you rather like a parameter for script to indicate the directory where > > the xml are? > Yeah, that would work. Or just instruct users to copy this file to a new > folder to use, and then copy the resulting file back out. In my mind, the parameter to indicate the directory where the xml are is much more handy. > > > > > > @@ +31,5 @@ > > > > +def fileGenerateHeader(f,fileDefine): > > > > + f.write('/* This Source Code Form is subject to the terms of the Mozilla Public\n') > > > > + f.write(' * License, v. 2.0. If a copy of the MPL was not distributed with this\n') > > > > + f.write(' * file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n') > > > > + f.write('\n') > > > > > > The nicer way to do this is generally: > > > f.write("\n".join([ > > > 'line1', > > > 'line2' > > > ])) > > I don't like what your are proposing because it would generate > > f.write('line1\nline2'); instead of f.write('line1\nline2\n'); > And the usual response is: > f.write('\n'.join([ > 'line1', > 'line2', > '' > ])) > > Alternatively, add a helper function: > def WriteLn(line): > f.write(line + '\n') > > WriteLn('\n'.join... Ok. > > > > > > @@ +89,5 @@ > > > > + self.vendors['ARB'] = 1 > > > > + self.vendors['EXT'] = 1 > > > > + > > > > + for lib in GLDataBase.LIBS: > > > > + self.libs[lib] = 1 > > > > > > Why are we setting these vendors and libs to one? > > I'm using a map to have a logarithmic complexity for checking if a word is a > > vendor. Therefore the key is important, and the value doesn't matter. That > > why I set to 1 for every one. [0] > A map without values associated with keys is just a set, so use a set. :) Oups... Fixed. > > > > > > @@ +93,5 @@ > > > > + self.libs[lib] = 1 > > > > + > > > > + > > > > + def loadXml(self,path): > > > > + > > > > > > Why a blank line after a `def:`? > > Why not? Let's keep a readable script instead of a highly concentrated > > script. [1] > This makes it less readable, in my opinion, and goes against the style we > use for c++. > Our styles for python and c++ shouldn't be wildly different, and also, other > short functions in this file don't follow this style. Ok. > > > > > > @@ +94,5 @@ > > > > + > > > > + > > > > + def loadXml(self,path): > > > > + > > > > + tree = xml.etree.ElementTree.parse(getScriptDir()+path) > > > > > > Spaces around operators. (` + `) > > Fixed. > > > Also, this should handle failure cases gracefully: What if someone forgets a > > > file? > > The python fail on xml.etree.ElementTree.parse(getScriptDir()+path), and > > GLConsts.h is not changed. > Does the error it give make it obvious what's wrong? Generally, catch the > exception, and give us a readable 'Error: source xml \'' + getScriptDir() + > path + '\' not found.'. I will check if the file exist before. > > > > > > @@ +98,5 @@ > > > > + tree = xml.etree.ElementTree.parse(getScriptDir()+path) > > > > + root = tree.getroot() > > > > + > > > > + for enums in root.iter('enums'): > > > > + > > > > > > Why have a blank line after `for:`? > > [1] > > > > > > @@ +101,5 @@ > > > > + for enums in root.iter('enums'): > > > > + > > > > + vendor = enums.get('vendor') > > > > + if not vendor: > > > > + vendor = 'ARB' > > > > > > Why do we set it to ARB? > > OpenGL 1.0 features don't have official vendors, for historical reasons. So > > because this is in a 'Core profile' I fake it as ARB vendor. > Why? It is the way it is ... some specified constants don't have any vendor, so I fake it to ARB and it works well. > > > > > > @@ +104,5 @@ > > > > + if not vendor: > > > > + vendor = 'ARB' > > > > + > > > > + if vendor not in self.vendors: > > > > + self.vendors[vendor] = 1 > > > > > > What does `1` mean here? > > [0] > Same here. > > > > > > @@ +132,5 @@ > > > > + > > > > + > > > > + def getConstValue(self,const): > > > > + > > > > + searchName = 'GL_' + const.name > > > > > > Shouldn't we use const.lib instead of always 'GL_'? > > Librairies are rendered in GLConsts.h by the order indicated in > > GLDatabase.LIBS. So this function, just try to find a equivalent consts in > > the multiplatform library 'GL', even for other libraries. For example, that > > let us have: > > #define LOCAL_EGL_FALSE LOCAL_GL_FALSE > > instead of: > > #define LOCAL_EGL_FALSE 0 > > Which is beater for readability because we clearly see that LOCAL_EGL_FALSE > > and LOCAL_GL_FALSE are compatible. > I don't know as I like this. Different libraries deliberately have different > namespaces. Removed. > > Also, s/Xml/XML/. Ok.

Comment on attachment 787189 [details] [diff] [review] patch revision 8 (r+ by bjacob and jrmuizel) Review of attachment 787189 [details] [diff] [review]: ----------------------------------------------------------------- This should definitely be two patches: One to add the pars One to add the resulting files For flexibility, I would also keep three functions at the top of GLParseRegistryXML.py: FormatLibBegin(lib) FormatLibEnd(lib) FormatConstant(lib, const, vendor) That way, we can bikeshed about just those functions, and leave the rest of the implementation alone. ::: gfx/gl/GLParseRegistryXML.py @@ +1,5 @@ > +#!/usr/bin/env python > +# coding=utf8 > + > + > +####################################################################### TUTORIAL Don't put text this far out to the right. Put it on the next line. @@ +25,5 @@ > +import xml.etree.ElementTree > + > + > + > + I'm drowning in whitespace here. :P @@ +31,5 @@ > + return os.path.dirname(__file__) + '/' > + > + > +def getXMLDir(): > + if len(sys.argv) == 1: Zero never happens, but <2 is better than ==1, since that's really what we care about. @@ +34,5 @@ > +def getXMLDir(): > + if len(sys.argv) == 1: > + return './' > + > + if sys.argv[1][-1] == '/': You use `sys.argv[1]` three times, so pull it out into a variable. Anything that's [1][-1] is a little disconcerting to parse. @@ +37,5 @@ > + > + if sys.argv[1][-1] == '/': > + return sys.argv[1] > + > + return sys.argv[1] + '/' It's better to add a slash if it doesn't have one, so we have one point we return from. @@ +77,5 @@ > + ]) > + > + > +def getGLType(type): > + Whitespace after the define line looks disjointed, and I've been unable to find a python style guide which recommends this. @@ +120,5 @@ > + > + XMLpath = getXMLDir() + path > + > + if not os.path.isfile(XMLpath): > + print 'missing file "' + XMLpath + '"' The pythonic thing to do is to raise an exception. This should be as easy as `raise Exception('Missing file: ' + XMLPath)`. CamelCase with abbreviations is always sort of iffy. `XMLPath` is probably better than `XMLpath`, but `xmlPath` is probably better still. @@ +137,5 @@ > + if vendor not in self.vendors: > + # we map this new vendor in the vendors set. > + self.vendors.add(vendor) > + > + namespacetype = enums.get('type') namespaceType @@ +158,5 @@ > + if not type: > + # if no type specified, we get the namespace's default type > + type = namespacetype > + > + self.consts[lib + '_' + name] = GLConst(lib, name, value, type) It seems strange that we rebuild `lib + '_' + name` when that's what we had to begin with. @@ +163,5 @@ > + > + return True > + > + > + def canSkeepConst(self, const): 'Skeep'? 'Skip'? @@ +176,5 @@ > + if nameSuffix not in self.vendors: > + # this constant is a standardized one > + return False > + > + searchName = searchName[0:-len(nameSuffix) - 1] Make this more explicit with `-(len(nameSuffix) + 1)` @@ +192,5 @@ > + > + def constDefinition(self, const): > + line = '#define ' + const.access() > + whiteSpace = max(60 - len(line), 0) > + return line + ' ' * whiteSpace + ' ' + const.value To keep order of operations obvious, consider `line + ' '*whitespace + ' ' + const.value` Also, consider adding one to `whitespace` above, since that's what we end up doing here anyways. 'whitespace' is also one word, so don't camelCase the 's'. :) @@ +196,5 @@ > + return line + ' ' * whiteSpace + ' ' + const.value > + > + > + def exportConsts(self, path): > + f = open(getScriptDir() + path,'w') This can fail, even if it's unlikely. (no write privs, or some such) Catch the exception and raise an exception of our own which tells us something useful. @@ +201,5 @@ > + > + fileDefine = 'GLCONSTS' > + fileGenerateHeader(f, fileDefine) > + > + fileWrite(f, 1) I don't love overloading this function to do this, but in python, it's sort of ok. (Even still, the usual implementation of an int overload for a write function is to stringify the int, and write that. I recommend making a different function for emitting newlines. `writeNewlines`? @@ +213,5 @@ > + for constName in constNames: > + const = self.consts[constName] > + > + if const.lib != lib: > + continue It would probably have been better to keep a map of lib=>constMap, design-wise, and also bringing this down to O(n) instead of O(k*n). @@ +226,5 @@ > + fileGenerateFooter(f, fileDefine) > + > + f.close() > + > + return True Don't return anything if we never check it, and have no fail condition. (though I am asking you to add one above!) Python would have us use exceptions for error handling anyways. @@ +232,5 @@ > + > +glDatabase = GLDatabase() > + > +success = glDatabase.loadXML('gl.xml') > +success = success and glDatabase.loadXML('egl.xml') Using exceptions helps prevent this sort of madness. :)

(In reply to Jeff Gilbert [:jgilbert] from comment #19) > Comment on attachment 787189 [details] [diff] [review] > patch revision 8 (r+ by bjacob and jrmuizel) > > Review of attachment 787189 [details] [diff] [review]: > ----------------------------------------------------------------- > > This should definitely be two patches: > One to add the pars > One to add the resulting files > > For flexibility, I would also keep three functions at the top of > GLParseRegistryXML.py: > FormatLibBegin(lib) > FormatLibEnd(lib) > FormatConstant(lib, const, vendor) > > That way, we can bikeshed about just those functions, and leave the rest of > the implementation alone. > > ::: gfx/gl/GLParseRegistryXML.py > @@ +1,5 @@ > > +#!/usr/bin/env python > > +# coding=utf8 > > + > > + > > +####################################################################### TUTORIAL > > Don't put text this far out to the right. Put it on the next line. -_-' > > @@ +25,5 @@ > > +import xml.etree.ElementTree > > + > > + > > + > > + > > I'm drowning in whitespace here. :P No comments... > > @@ +31,5 @@ > > + return os.path.dirname(__file__) + '/' > > + > > + > > +def getXMLDir(): > > + if len(sys.argv) == 1: > > Zero never happens, but <2 is better than ==1, since that's really what we > care about. Seriously?! > > @@ +34,5 @@ > > +def getXMLDir(): > > + if len(sys.argv) == 1: > > + return './' > > + > > + if sys.argv[1][-1] == '/': > > You use `sys.argv[1]` three times, so pull it out into a variable. Anything > that's [1][-1] is a little disconcerting to parse. Talking of performance of a python script is a wast of time. > > @@ +37,5 @@ > > + > > + if sys.argv[1][-1] == '/': > > + return sys.argv[1] > > + > > + return sys.argv[1] + '/' > > It's better to add a slash if it doesn't have one, so we have one point we > return from. No. because I don't add a new '/' at the end if there is already one. > > @@ +77,5 @@ > > + ]) > > + > > + > > +def getGLType(type): > > + > > Whitespace after the define line looks disjointed, and I've been unable to > find a python style guide which recommends this. Fixed. > > @@ +120,5 @@ > > + > > + XMLpath = getXMLDir() + path > > + > > + if not os.path.isfile(XMLpath): > > + print 'missing file "' + XMLpath + '"' > > The pythonic thing to do is to raise an exception. > This should be as easy as `raise Exception('Missing file: ' + XMLPath)`. > > CamelCase with abbreviations is always sort of iffy. `XMLPath` is probably > better than `XMLpath`, but `xmlPath` is probably better still. Fixed. > > @@ +137,5 @@ > > + if vendor not in self.vendors: > > + # we map this new vendor in the vendors set. > > + self.vendors.add(vendor) > > + > > + namespacetype = enums.get('type') > > namespaceType Oups... > > @@ +158,5 @@ > > + if not type: > > + # if no type specified, we get the namespace's default type > > + type = namespacetype > > + > > + self.consts[lib + '_' + name] = GLConst(lib, name, value, type) > > It seems strange that we rebuild `lib + '_' + name` when that's what we had > to begin with. Explicit generation of the primary key for readability. > > @@ +163,5 @@ > > + > > + return True > > + > > + > > + def canSkeepConst(self, const): > > 'Skeep'? 'Skip'? Oups... > > @@ +176,5 @@ > > + if nameSuffix not in self.vendors: > > + # this constant is a standardized one > > + return False > > + > > + searchName = searchName[0:-len(nameSuffix) - 1] > > Make this more explicit with `-(len(nameSuffix) + 1)` Seriously!... This is same! > > @@ +192,5 @@ > > + > > + def constDefinition(self, const): > > + line = '#define ' + const.access() > > + whiteSpace = max(60 - len(line), 0) > > + return line + ' ' * whiteSpace + ' ' + const.value > > To keep order of operations obvious, consider `line + ' '*whitespace + ' ' + > const.value` > Also, consider adding one to `whitespace` above, since that's what we end up > doing here anyways. > > 'whitespace' is also one word, so don't camelCase the 's'. :) Oups... > > @@ +196,5 @@ > > + return line + ' ' * whiteSpace + ' ' + const.value > > + > > + > > + def exportConsts(self, path): > > + f = open(getScriptDir() + path,'w') > > This can fail, even if it's unlikely. (no write privs, or some such) Catch > the exception and raise an exception of our own which tells us something > useful. Seriously, what is the probability to have a exception here, even, how many times this script will be executed? > > @@ +201,5 @@ > > + > > + fileDefine = 'GLCONSTS' > > + fileGenerateHeader(f, fileDefine) > > + > > + fileWrite(f, 1) > > I don't love overloading this function to do this, but in python, it's sort > of ok. (Even still, the usual implementation of an int overload for a write > function is to stringify the int, and write that. > > I recommend making a different function for emitting newlines. > `writeNewlines`? No comments... > > @@ +213,5 @@ > > + for constName in constNames: > > + const = self.consts[constName] > > + > > + if const.lib != lib: > > + continue > > It would probably have been better to keep a map of lib=>constMap, > design-wise, and also bringing this down to O(n) instead of O(k*n). K=4 so that still a O(n). And this script will be maybe executed every year for new OpenGL release? We have more important to do than optimizing that... And this is a O(n*log(n)) by the way. > > @@ +226,5 @@ > > + fileGenerateFooter(f, fileDefine) > > + > > + f.close() > > + > > + return True > > Don't return anything if we never check it, and have no fail condition. > (though I am asking you to add one above!) Python would have us use > exceptions for error handling anyways. [1] So what do you want? The same guideline as C++, or not? > > @@ +232,5 @@ > > + > > +glDatabase = GLDatabase() > > + > > +success = glDatabase.loadXML('gl.xml') > > +success = success and glDatabase.loadXML('egl.xml') > > Using exceptions helps prevent this sort of madness. :) [1] This is not madness! This is explicit and readable error handling! I think that review is going mostly to far...

(In reply to Guillaume Abadie from comment #20) > (In reply to Jeff Gilbert [:jgilbert] from comment #19) > > Comment on attachment 787189 [details] [diff] [review] > > patch revision 8 (r+ by bjacob and jrmuizel) > > > > Review of attachment 787189 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This should definitely be two patches: > > One to add the pars > > One to add the resulting files > > > > For flexibility, I would also keep three functions at the top of > > GLParseRegistryXML.py: > > FormatLibBegin(lib) > > FormatLibEnd(lib) > > FormatConstant(lib, const, vendor) > > > > That way, we can bikeshed about just those functions, and leave the rest of > > the implementation alone. > > > > > > @@ +34,5 @@ > > > +def getXMLDir(): > > > + if len(sys.argv) == 1: > > > + return './' > > > + > > > + if sys.argv[1][-1] == '/': > > > > You use `sys.argv[1]` three times, so pull it out into a variable. Anything > > that's [1][-1] is a little disconcerting to parse. > Talking of performance of a python script is a wast of time. This is not a perf concern, it is a readability concern. > > > > @@ +37,5 @@ > > > + > > > + if sys.argv[1][-1] == '/': > > > + return sys.argv[1] > > > + > > > + return sys.argv[1] + '/' > > > > It's better to add a slash if it doesn't have one, so we have one point we > > return from. > No. because I don't add a new '/' at the end if there is already one. I mean like: dirPath = sys.argv[1] if dirPath[-1] != '/': dirPath += '/' return dirPath This way, the code reads just like what it does. Get dirPath from the second arg in the argv. If dirPath doesn't end in a slash, add a slash. Return dirPath. > > > > @@ +158,5 @@ > > > + if not type: > > > + # if no type specified, we get the namespace's default type > > > + type = namespacetype > > > + > > > + self.consts[lib + '_' + name] = GLConst(lib, name, value, type) > > > > It seems strange that we rebuild `lib + '_' + name` when that's what we had > > to begin with. > Explicit generation of the primary key for readability. Alright, sounds good. > > > > @@ +176,5 @@ > > > + if nameSuffix not in self.vendors: > > > + # this constant is a standardized one > > > + return False > > > + > > > + searchName = searchName[0:-len(nameSuffix) - 1] > > > > Make this more explicit with `-(len(nameSuffix) + 1)` > Seriously!... This is same! `foo ^= foo` is the same as `foo = 0`, but we use the latter, not the former. In general, we shouldn't be doing math inline in array indexing. If we are, we should make sure it's very obvious exactly what we're doing. In this case, it's the composition of two actions: Calculate the `tailLength` of what we want to remove. Slice off the tail with `[:-tailLength]`. My suggestion formats it so it's obvious what we're trying to do, given understanding of python's `[:-foo]` meme. > > > > @@ +196,5 @@ > > > + return line + ' ' * whiteSpace + ' ' + const.value > > > + > > > + > > > + def exportConsts(self, path): > > > + f = open(getScriptDir() + path,'w') > > > > This can fail, even if it's unlikely. (no write privs, or some such) Catch > > the exception and raise an exception of our own which tells us something > > useful. > Seriously, what is the probability to have a exception here, even, how many > times this script will be executed? Neglecting to handle these cases is what leads to really inscrutable errors for people-who-are-not-the-author down the road. > > > > @@ +201,5 @@ > > > + > > > + fileDefine = 'GLCONSTS' > > > + fileGenerateHeader(f, fileDefine) > > > + > > > + fileWrite(f, 1) > > > > I don't love overloading this function to do this, but in python, it's sort > > of ok. (Even still, the usual implementation of an int overload for a write > > function is to stringify the int, and write that. > > > > I recommend making a different function for emitting newlines. > > `writeNewlines`? > No comments... > > > > @@ +213,5 @@ > > > + for constName in constNames: > > > + const = self.consts[constName] > > > + > > > + if const.lib != lib: > > > + continue > > > > It would probably have been better to keep a map of lib=>constMap, > > design-wise, and also bringing this down to O(n) instead of O(k*n). > K=4 so that still a O(n). And this script will be maybe executed every year > for new OpenGL release? We have more important to do than optimizing that... > And this is a O(n*log(n)) by the way. Again, a design concern, and a perf note-of-interest. I care more about design here unless we believe perf to matter. (Which we shouldn't here) > > > > @@ +226,5 @@ > > > + fileGenerateFooter(f, fileDefine) > > > + > > > + f.close() > > > + > > > + return True > > > > Don't return anything if we never check it, and have no fail condition. > > (though I am asking you to add one above!) Python would have us use > > exceptions for error handling anyways. > [1] So what do you want? The same guideline as C++, or not? It's not either or. Python is a different language. Exceptions are something we *can't* use in gecko's c++, but we can (and should) use them in python, since that's how python is designed to be written. However, when we have no guidance for the python way to do something, either from standard python style or some mozilla python style guide, we should fall back on what will be most familiar to the people who will touch this code: Mozilla c++ devs. > > > > @@ +232,5 @@ > > > + > > > +glDatabase = GLDatabase() > > > + > > > +success = glDatabase.loadXML('gl.xml') > > > +success = success and glDatabase.loadXML('egl.xml') > > > > Using exceptions helps prevent this sort of madness. :) > [1] This is not madness! This is explicit and readable error handling! But what went wrong? It's very different to know that *an* error occurred, vs *which* error. This tells us: "An error happened!" What we could have: "Failed to read from file egl.xml!" Though, here, you have `print`s to display the errors. Still, > > I think that review is going mostly to far... "It seems to work" isn't good enough if we're going to be relying on, and maintaining it. Not every review comment I make mandates a change: Some are just food for thought, and others are questions about why some other approach wasn't tried. Less specifically, reviews are when we can look at a solution and have a conversation about how or if it can be improved. I think the current revision indeed looks much better than the initial proposal, so mission accomplished.

(In reply to Jeff Gilbert [:jgilbert] from comment #23) > (In reply to Guillaume Abadie from comment #20) > > (In reply to Jeff Gilbert [:jgilbert] from comment #19) > > > Comment on attachment 787189 [details] [diff] [review] > > > patch revision 8 (r+ by bjacob and jrmuizel) > > > > > > Review of attachment 787189 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > This should definitely be two patches: > > > One to add the pars > > > One to add the resulting files > > > > > > For flexibility, I would also keep three functions at the top of > > > GLParseRegistryXML.py: > > > FormatLibBegin(lib) > > > FormatLibEnd(lib) > > > FormatConstant(lib, const, vendor) > > > > > > That way, we can bikeshed about just those functions, and leave the rest of > > > the implementation alone. > > > > > > > > > @@ +34,5 @@ > > > > +def getXMLDir(): > > > > + if len(sys.argv) == 1: > > > > + return './' > > > > + > > > > + if sys.argv[1][-1] == '/': > > > > > > You use `sys.argv[1]` three times, so pull it out into a variable. Anything > > > that's [1][-1] is a little disconcerting to parse. > > Talking of performance of a python script is a wast of time. > This is not a perf concern, it is a readability concern. > > > > > > @@ +37,5 @@ > > > > + > > > > + if sys.argv[1][-1] == '/': > > > > + return sys.argv[1] > > > > + > > > > + return sys.argv[1] + '/' > > > > > > It's better to add a slash if it doesn't have one, so we have one point we > > > return from. > > No. because I don't add a new '/' at the end if there is already one. > I mean like: > dirPath = sys.argv[1] > if dirPath[-1] != '/': > dirPath += '/' > > return dirPath > > This way, the code reads just like what it does. > Get dirPath from the second arg in the argv. > If dirPath doesn't end in a slash, add a slash. > Return dirPath. Fair enough. Fixed. > > > > > > @@ +158,5 @@ > > > > + if not type: > > > > + # if no type specified, we get the namespace's default type > > > > + type = namespacetype > > > > + > > > > + self.consts[lib + '_' + name] = GLConst(lib, name, value, type) > > > > > > It seems strange that we rebuild `lib + '_' + name` when that's what we had > > > to begin with. > > Explicit generation of the primary key for readability. > Alright, sounds good. > > > > > > @@ +176,5 @@ > > > > + if nameSuffix not in self.vendors: > > > > + # this constant is a standardized one > > > > + return False > > > > + > > > > + searchName = searchName[0:-len(nameSuffix) - 1] > > > > > > Make this more explicit with `-(len(nameSuffix) + 1)` > > Seriously!... This is same! > `foo ^= foo` is the same as `foo = 0`, but we use the latter, not the former. > In general, we shouldn't be doing math inline in array indexing. If we are, > we should > make sure it's very obvious exactly what we're doing. In this case, it's the > composition > of two actions: > Calculate the `tailLength` of what we want to remove. > Slice off the tail with `[:-tailLength]`. > > My suggestion formats it so it's obvious what we're trying to do, given > understanding of python's `[:-foo]` meme. Already fixed. > > > > > > @@ +196,5 @@ > > > > + return line + ' ' * whiteSpace + ' ' + const.value > > > > + > > > > + > > > > + def exportConsts(self, path): > > > > + f = open(getScriptDir() + path,'w') > > > > > > This can fail, even if it's unlikely. (no write privs, or some such) Catch > > > the exception and raise an exception of our own which tells us something > > > useful. > > Seriously, what is the probability to have a exception here, even, how many > > times this script will be executed? > Neglecting to handle these cases is what leads to really inscrutable errors > for people-who-are-not-the-author down the road. > > > > > > @@ +201,5 @@ > > > > + > > > > + fileDefine = 'GLCONSTS' > > > > + fileGenerateHeader(f, fileDefine) > > > > + > > > > + fileWrite(f, 1) > > > > > > I don't love overloading this function to do this, but in python, it's sort > > > of ok. (Even still, the usual implementation of an int overload for a write > > > function is to stringify the int, and write that. > > > > > > I recommend making a different function for emitting newlines. > > > `writeNewlines`? > > No comments... > > > > > > @@ +213,5 @@ > > > > + for constName in constNames: > > > > + const = self.consts[constName] > > > > + > > > > + if const.lib != lib: > > > > + continue > > > > > > It would probably have been better to keep a map of lib=>constMap, > > > design-wise, and also bringing this down to O(n) instead of O(k*n). > > K=4 so that still a O(n). And this script will be maybe executed every year > > for new OpenGL release? We have more important to do than optimizing that... > > And this is a O(n*log(n)) by the way. > Again, a design concern, and a perf note-of-interest. I care more about > design here unless we believe perf to matter. (Which we shouldn't here) I don't know where is the problem to do only one primary key, and then just do like several SQL like requests : select * from consts where lib = 'GL' for each library > > > > > > > @@ +226,5 @@ > > > > + fileGenerateFooter(f, fileDefine) > > > > + > > > > + f.close() > > > > + > > > > + return True > > > > > > Don't return anything if we never check it, and have no fail condition. > > > (though I am asking you to add one above!) Python would have us use > > > exceptions for error handling anyways. > > [1] So what do you want? The same guideline as C++, or not? > It's not either or. Python is a different language. Exceptions are something > we *can't* use in gecko's c++, but we can (and should) use them in python, > since that's how python is designed to be written. > > However, when we have no guidance for the python way to do something, either > from standard python style or some mozilla python style guide, we should > fall back on what will be most familiar to the people who will touch this > code: Mozilla c++ devs. Well, as you said, here it is. > > > > > > @@ +232,5 @@ > > > > + > > > > +glDatabase = GLDatabase() > > > > + > > > > +success = glDatabase.loadXML('gl.xml') > > > > +success = success and glDatabase.loadXML('egl.xml') > > > > > > Using exceptions helps prevent this sort of madness. :) > > [1] This is not madness! This is explicit and readable error handling! > But what went wrong? It's very different to know that *an* error occurred, > vs *which* error. > This tells us: "An error happened!" > What we could have: "Failed to read from file egl.xml!" > > Though, here, you have `print`s to display the errors. Still, Why does the caller need to know the reason of the failure, while it would not be able to work around anyway? This program is a command line tool, the print is enough. > > > > I think that review is going mostly to far... > "It seems to work" isn't good enough if we're going to be relying on, and > maintaining it. > Not every review comment I make mandates a change: Some are just food for > thought, and others are questions about why some other approach wasn't > tried. Less specifically, reviews are when we can look at a solution and > have a conversation about how or if it can be improved. I think the current > revision indeed looks much better than the initial proposal, so mission > accomplished.

Don't need to actualize step 2 since this is generated file.

(In reply to Guillaume Abadie from comment #24) > (In reply to Jeff Gilbert [:jgilbert] from comment #23) > > (In reply to Guillaume Abadie from comment #20) > > > (In reply to Jeff Gilbert [:jgilbert] from comment #19) > > > > Comment on attachment 787189 [details] [diff] [review] > > > > patch revision 8 (r+ by bjacob and jrmuizel) > > > > > > > > Review of attachment 787189 [details] [diff] [review]: > > > > ----------------------------------------------------------------- > > > > > > > > @@ +213,5 @@ > > > > > + for constName in constNames: > > > > > + const = self.consts[constName] > > > > > + > > > > > + if const.lib != lib: > > > > > + continue > > > > > > > > It would probably have been better to keep a map of lib=>constMap, > > > > design-wise, and also bringing this down to O(n) instead of O(k*n). > > > K=4 so that still a O(n). And this script will be maybe executed every year > > > for new OpenGL release? We have more important to do than optimizing that... > > > And this is a O(n*log(n)) by the way. > > Again, a design concern, and a perf note-of-interest. I care more about > > design here unless we believe perf to matter. (Which we shouldn't here) > I don't know where is the problem to do only one primary key, and then just > do like several SQL like requests : select * from consts where lib = 'GL' > for each library It's just not as clean as having things already separated out the way we want them. (Split by library) > > > > > > > > > > @@ +226,5 @@ > > > > > + fileGenerateFooter(f, fileDefine) > > > > > + > > > > > + f.close() > > > > > + > > > > > + return True > > > > > > > > Don't return anything if we never check it, and have no fail condition. > > > > (though I am asking you to add one above!) Python would have us use > > > > exceptions for error handling anyways. > > > [1] So what do you want? The same guideline as C++, or not? > > It's not either or. Python is a different language. Exceptions are something > > we *can't* use in gecko's c++, but we can (and should) use them in python, > > since that's how python is designed to be written. > > > > However, when we have no guidance for the python way to do something, either > > from standard python style or some mozilla python style guide, we should > > fall back on what will be most familiar to the people who will touch this > > code: Mozilla c++ devs. > Well, as you said, here it is. I'm not sure what you mean by that. Using exceptions for handling errors is the pythonic way to do things. We definitely have guidance in this regard. > > > > > > > > @@ +232,5 @@ > > > > > + > > > > > +glDatabase = GLDatabase() > > > > > + > > > > > +success = glDatabase.loadXML('gl.xml') > > > > > +success = success and glDatabase.loadXML('egl.xml') > > > > > > > > Using exceptions helps prevent this sort of madness. :) > > > [1] This is not madness! This is explicit and readable error handling! > > But what went wrong? It's very different to know that *an* error occurred, > > vs *which* error. > > This tells us: "An error happened!" > > What we could have: "Failed to read from file egl.xml!" > > > > Though, here, you have `print`s to display the errors. Still, > Why does the caller need to know the reason of the failure, while it would > not be able to work around anyway? This program is a command line tool, the > print is enough. It should very rarely be the callee's choice what the caller should do on failure. To this end, the callee should indicate what went wrong, and the caller has a choice of trying to recover, or not. Python makes this really easy to do. FWIW, a command line tool should not just print an error, but also have a return code reflecting its success or failure. Exceptions also give us this for free, among other things.

Comment on attachment 789312 [details] [diff] [review] patch revision 10 - step 1 (r+ by bjacob and jrmuizel) Review of attachment 789312 [details] [diff] [review]: ----------------------------------------------------------------- Anything that can return a value should be checked and/or asserted against, unless otherwise noted. ('we don't care about whether this succeeds') Things should not return true if they can never return false. ::: gfx/gl/GLParseRegistryXML.py @@ +242,5 @@ > + fileHeader.formatFileEnd() > + > + f.close() > + > + return True Don't return true if failure isn't possible.

Comment on attachment 787680 [details] [diff] [review] patch revision 9 - step 1 (r+ by bjacob and jrmuizel) Review of attachment 787680 [details] [diff] [review]: ----------------------------------------------------------------- Remove the dead code and add some space seps. ::: gfx/gl/GLParseRegistryXML.py @@ +37,5 @@ > + > + def write(self, arg): > + if isinstance(arg, list): > + self.f.write('\n'.join(arg) + '\n') > + elif isinstance(arg,(int, long)): Space after commas in function arg lists. ([1]) @@ +44,5 @@ > + self.f.write(str(arg) + '\n') > + > + > + def formatLibBegin(self, lib): > + #lib would be 'GL', 'EGL', 'GLX' or 'WGL' Space between comment marker and comment: # foo bar @@ +80,5 @@ > + > + return sys.argv[1] + '/' > + > + > +def fileWrite(f, arg): It would be nice to move this up before `GLConstHeader.write` for reuse up there. @@ +83,5 @@ > + > +def fileWrite(f, arg): > + if isinstance(arg, list): > + f.write('\n'.join(arg) + '\n') > + elif isinstance(arg,(int, long)): [1] @@ +100,5 @@ > + '#define ' + fileDefine + '_H_', > + '', > + '/**', > + ' * GENERATED FILE, DO NOT MODIFY.', > + ' * THIS IS A GENERATED FILE DIRECTLY FROM THE OFFCIAL OPENGL REGISTRY', I would say 'DO NOT MODIFY DIRECTLY.'. "This file is generated directly from the official OpenGL registry XML docs." @@ +117,5 @@ > + ]) > + > + > +def getGLType(type): > + if type=="bitmask": Spaces around operators. @@ +148,5 @@ > + # so we manualy declare them > + > + > + def loadXML(self, path): > + Unnecessary newline. @@ +196,5 @@ > + return True > + > + > + def canSkipConst(self, const): > + # don't skeep any const for now. s/skeep/skip/ Wait, we're not using this? Remove the code below if we're not using it. If you want to keep a record of it, attach it as a new patch to this or another bug. @@ +222,5 @@ > + return True > + > + > + def exportConsts(self, path): > + f = open(getScriptDir() + path,'w') [1] @@ +229,5 @@ > + fileGenerateHeader(f, fileDefine) > + > + fileWrite(f, 1) > + > + fileHeader = GLConstHeader(f) `headerFile` would be better, since this is the formatter for the header file, not the header for the file. @@ +254,5 @@ > + fileGenerateFooter(f, fileDefine) > + > + f.close() > + > + return True Don't return a status if the function can't fail.

(In reply to Jeff Gilbert [:jgilbert] from comment #27) > Comment on attachment 789312 [details] [diff] [review] > patch revision 10 - step 1 (r+ by bjacob and jrmuizel) > > Review of attachment 789312 [details] [diff] [review]: > ----------------------------------------------------------------- > > Anything that can return a value should be checked and/or asserted against, > unless otherwise noted. ('we don't care about whether this succeeds') > Things should not return true if they can never return false. > > ::: gfx/gl/GLParseRegistryXML.py > @@ +242,5 @@ > > + fileHeader.formatFileEnd() > > + > > + f.close() > > + > > + return True > > Don't return true if failure isn't possible. Fixed. I also used with open(getScriptDir() + path,'w') as f:. Thanks!

(In reply to Jeff Gilbert [:jgilbert] from comment #28) > Comment on attachment 787680 [details] [diff] [review] > patch revision 9 - step 1 (r+ by bjacob and jrmuizel) > > Review of attachment 787680 [details] [diff] [review]: > ----------------------------------------------------------------- > > Remove the dead code and add some space seps. Done! > > ::: gfx/gl/GLParseRegistryXML.py > @@ +37,5 @@ > > + > > + def write(self, arg): > > + if isinstance(arg, list): > > + self.f.write('\n'.join(arg) + '\n') > > + elif isinstance(arg,(int, long)): > > Space after commas in function arg lists. ([1]) Oups... > > @@ +44,5 @@ > > + self.f.write(str(arg) + '\n') > > + > > + > > + def formatLibBegin(self, lib): > > + #lib would be 'GL', 'EGL', 'GLX' or 'WGL' > > Space between comment marker and comment: # foo bar Fixed. > > @@ +80,5 @@ > > + > > + return sys.argv[1] + '/' > > + > > + > > +def fileWrite(f, arg): > > It would be nice to move this up before `GLConstHeader.write` for reuse up > there. Fixed. > > @@ +83,5 @@ > > + > > +def fileWrite(f, arg): > > + if isinstance(arg, list): > > + f.write('\n'.join(arg) + '\n') > > + elif isinstance(arg,(int, long)): > > [1] Fixed. > > @@ +100,5 @@ > > + '#define ' + fileDefine + '_H_', > > + '', > > + '/**', > > + ' * GENERATED FILE, DO NOT MODIFY.', > > + ' * THIS IS A GENERATED FILE DIRECTLY FROM THE OFFCIAL OPENGL REGISTRY', > > I would say 'DO NOT MODIFY DIRECTLY.'. > "This file is generated directly from the official OpenGL registry XML docs." Fixed. > > @@ +117,5 @@ > > + ]) > > + > > + > > +def getGLType(type): > > + if type=="bitmask": > > Spaces around operators. Since we are planning for MOZ_GL_FOO_BAR, we don't need it anymore. > > @@ +148,5 @@ > > + # so we manualy declare them > > + > > + > > + def loadXML(self, path): > > + > > Unnecessary newline. Oups... > > @@ +196,5 @@ > > + return True > > + > > + > > + def canSkipConst(self, const): > > + # don't skeep any const for now. > > s/skeep/skip/ > Wait, we're not using this? Remove the code below if we're not using it. > > If you want to keep a record of it, attach it as a new patch to this or > another bug. Removed. > > @@ +222,5 @@ > > + return True > > + > > + > > + def exportConsts(self, path): > > + f = open(getScriptDir() + path,'w') > > [1] Fixed. > > @@ +229,5 @@ > > + fileGenerateHeader(f, fileDefine) > > + > > + fileWrite(f, 1) > > + > > + fileHeader = GLConstHeader(f) > > `headerFile` would be better, since this is the formatter for the header > file, not the header for the file. Fixed. > > @@ +254,5 @@ > > + fileGenerateFooter(f, fileDefine) > > + > > + f.close() > > + > > + return True > > Don't return a status if the function can't fail. Fixed.

Push to try: https://tbpl.mozilla.org/?tree=Try&rev=62f22fbabf32 Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a845e2bcd946 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d217987888

https://hg.mozilla.org/mozilla-central/rev/a845e2bcd946 https://hg.mozilla.org/mozilla-central/rev/a4d217987888

Comment on attachment 787681 [details] [diff] [review] patch revision 9 - step 2 (r+ by bjacob and jrmuizel) Review of attachment 787681 [details] [diff] [review]: ----------------------------------------------------------------- As we discussed, LOCAL_GL_ is here to stay, even if largely for hysterical raisins. (The only better and consistent possibility was MOZ_GL_, but that change was decided to not be worth the churn in code and blame) ::: gfx/gl/GLConsts.h @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#if !defined(GLCONSTS_H_) ifndef would be better @@ +6,5 @@ > +#define GLCONSTS_H_ > + > +/** > + * GENERATED FILE, DO NOT MODIFY. > + * THIS IS A GENERATED FILE DIRECTLY FROM THE OFFCIAL OPENGL REGISTRY 'official' s/is a generated file/is a file generated/ This line probably doesn't need to be all-caps, unless other generated files are like this. The important one was the previous line. @@ +176,5 @@ > +#define LOCAL_GL_ATC_RGB_AMD 0x8C92 > +#define LOCAL_GL_ATOMIC_COUNTER_BARRIER_BIT 0x00001000 > +#define LOCAL_GL_ATOMIC_COUNTER_BARRIER_BIT_EXT 0x00001000 > +#define LOCAL_GL_ATOMIC_COUNTER_BUFFER 0x92C0 > +#define LOCAL_GL_ATOMIC_COUNTER_BUFFER_ACTIVE_ATOMIC_COUNTERS 0x92C5 It be nicer if we pushed things out in multiples of 4/8/10/some spaces, for these too-long identifiers. A good follow up bug.