Hello all, I've just reviewed Dmitri's patch, and I am OK with it, as most of it is "aesthetic", and it fixes a couple of mistakes (e.g. non-virtual destructor on class inheriting QObject). This code grew very organically from out base from QSAS, where we don't have a clearly defined coding style, and I admit I didn't pay much attention to that, so why not choose Dmitri's :)
The only thing I would reproach in it is that although it arranged the brackets/spaces/etc., it messed up the indenting. I chose to use tabs rather than spaces, and I'm ok to change for the other way, but it's not consistent any more. Anyway, these are only very minor considerations. Although I will not have much time in the future for the maintenance of this driver - and not at all before mid-July - I plan a new update by the end of July, in which I will improve a few things, so I can incorporate any suggestion as long as it is not overwhelming. I will see how I can re-think the static objects issue. As for splitting qt.h into as many headers as there are classes, I chose not to do so to keep the "drivers" directory easier to read (1 header for the Qt drivers rather than 7), but I admit this is very debatable, so feel free to change that if you wish. Alban Alan W. Irwin wrote: > Hi Alban (off list): > > Could you respond on the plplot-devel mailing list to Dmitri's patch he just > sent to that list concerning the coding style of the qt device driver code? > I am afraid it is quite possible the small changes I introduced to the qt > device driver have added some style issues because of my lack of C++ > expertise. Of course, the best coding style is a matter of opinion, but > ideally you and Dmitri will be able to come to consensus on best style. > However, if there is a difference of opinion, I will follow yours. > Furthermore, I would be happy to test whatever style changes come out of > this discussion to make sure no regressions have been introduced by the > changes. > > To give you some more background on this style issue, I have forwarded below > what Dmitri sent me privately a few days ago. It includes some background > information on his choice of preferred coding style that he unfortunately > did not send to the list with his patch. I responded to his questions by > letting him know I would defer to your judgement which I am doing now. :-) > > Best wishes, > > Alan > __________________________ > Alan W. Irwin > > Astronomical research affiliation with Department of Physics and Astronomy, > University of Victoria (astrowww.phys.uvic.ca). > > Programming affiliations with the FreeEOS equation-of-state implementation > for stellar interiors (freeeos.sf.net); PLplot scientific plotting software > package (plplot.org); the libLASi project (unifont.org/lasi); the Loads of > Linux Links project (loll.sf.net); and the Linux Brochure Project > (lbproject.sf.net). > __________________________ > > Linux-powered Science > __________________________ > > ---------- Forwarded message ---------- > Date: Fri, 19 Jun 2009 20:15:52 +0300 > From: Dmitri Gribenko <griboz...@gmail.com> > To: Alan W. Irwin <ir...@beluga.phys.uvic.ca> > Subject: Qt driver problems/suggestions > > Hello Alan, > > After looking at the source, I found the following problems: > > 1. Inconsistent coding style, long hard-to-read expressions without > spacing around operators, c-style casts etc. I make it conform more > or less to Google C++ coding guidelines [1]. > > 2. Code uses two static global objects derived from QObject > (QtPLDriver::mutex and an instance of MasterHandler). This should be > avoided because construction order is not defined. Also, on my system > this leads to destructors of those objects being called two times. > Anyway, Qt and KDE developers say this is a bad thing [2] [3]. > Although KDE developers offer some sort of a macro that could mitigate > those problems, I think we could come up with a solution that creates > the object during plplot init and de-init. > > 3. qt.h has some things (for example, MasterHandler) that user's code > doesn't need to see. Also, to go along with Qt programming style, one > should place only one class declaration per header. > > I can try to fix it, but I know that Qt driver is under active > development so I decided to ask you first (maybe you have some local > copy that differs much from what is in svn). > > [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml > [2] http://qt.gitorious.org/qt/pages/CodingConventions > [3] http://techbase.kde.org/Policies/Library_Code_Policy#Static_Objects > > > ------------------------------------------------------------------------------ Are you an open source citizen? Join us for the Open Source Bridge conference! Portland, OR, June 17-19. Two days of sessions, one day of unconference: $250. Need another reason to go? 24-hour hacker lounge. Register today! http://ad.doubleclick.net/clk;215844324;13503038;v?http://opensourcebridge.org _______________________________________________ Plplot-devel mailing list Plplot-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/plplot-devel
