From: Keith M. <kei...@us...> - 2006-05-05 19:35:31
|
Moving from mingw-users to mingw-dvlpr, where it's more appropriate. On Friday 05 May 2006 1:22 pm, Earnie Boyd wrote: > Quoting Brian Dessent <br...@de...>: > > This recent w32api commit: > > > > * include/winuser.h (MOUSEEVENTF_*): Cleanup and new define. > > (HSHELL_*): Cleanup and new define. > > (SMTO_*): Cleanup and new define. > > (SPI_*): Cleanup and new define. > > (SPIF_*): Cleanup. > > (HWND_MESSAGE): Windows 2000 only. > > (SIF_*): Cleanup. > > (SWP_*): Cleanup. > > * include/wingdi.h (ETO_*): Cleanup and new define. > > > > <http://sourceware.org/ml/cygwin-cvs/2006-q2/msg00090.html> As the new kid on the admin block, I'm a bit reticent about throwing my hat into this ring; however, IMO this style of ChangeLog entry, with asterisks replacing swathes of arbitrary and unspecified symbol names, should not be tolerated. It is explicitly discouraged by the GNU Coding Standards, on which our ChangeLog style is modelled; indeed, it could be argued that it is implicitly forbidden. While I appreciate that it may require additional effort to fully and explicitly itemise every symbol affected by a patch, this complaint from Brian clearly illustrates the reason why we should do so... > > ...seems to have broken the ability to compile w32api. Specifically, > > the #define of SPI_SCREENSAVERRUNNING is no longer present which > > causes the build of scrnsave.c to fail: Ok. Seems like the patch deleted the symbol definition for SPI_SCREENSAVERRUNNING. The problem is, I cannot determine this from the ChangeLog entry, and this is unacceptable. Furthermore, as this diff shows: > -#define SPI_SCREENSAVERRUNNING 97 > : > +#define SPI_GETSCREENSAVERRUNNING 0x0072 > : > +#define SPI_SETSCREENSAVERRUNNING 0x0061 the definition of SPI_SCREENSAVERRUNNING has been replaced by the pair SPI_GETSCREENSAVERRUNNING and SPI_SETSCREENSAVERRUNNING, but again, this is not evident from inspection of the ChangeLog. What the ChangeLog *should* say is something like: * include/winuser.h (SPI_SCREENSAVERRUNNING): Delete; replaced by... (SPI_GETSCREENSAVERRUNNING, SPI_SETSCREENSAVERRUNNING): these; define. : : and similarly for *all* other affected symbols. Dimitri, may I ask you please, to modify your ChangeLog entry to fully identify *every* symbol you've added, modified, or deleted? Regards, Keith. |
From: Dimitri Papadopoulos-O. <dim...@ce...> - 2006-05-10 07:21:44
|
Hi, Earnie Boyd wrote: >> So the explanations have to be put in the ChangeLog. Unless... Since >> GCC 4 supports precompiled headers, would it be possible to add some >> minimal comments in the header files? I was thinking of a mere link to >> the relevant documentation, something like: >> //http://msdn.microsoft.com/library/.../directshowstructures.asp >> [...] > In theory this sounds good but in practice it will quickly become a > nightmare. MSDN changes its URL references so that you have to continue > to search for the documentation. It isn't feasible in the case of > w32api documentation. OK, the URLs may change but the overall structure of the documentation can't change that often. What if I add the title of the page in addition to / instead of its URL? Something like: //Microsoft DirectShow 9.0 - DirectShow Structures or: //Microsoft DirectShow 9.0 - DirectShow Structures //http://msdn.microsoft.com/library/.../directshowstructures.asp How often do the URLs change? If they change once in a few years, then I think it's still useful to add the URLs. URLs that have not been valid for months may be a sign that the relevant header needs to be updated... Dimitri Papadopoulos |
From: Earnie B. <ea...@us...> - 2006-05-10 11:23:29
|
Quoting Dimitri Papadopoulos-Orfanos <dim...@ce...>: > Hi, > > Earnie Boyd wrote: >>> So the explanations have to be put in the ChangeLog. Unless... >>> Since GCC 4 supports precompiled headers, would it be possible to >>> add some minimal comments in the header files? I was thinking of a >>> mere link to the relevant documentation, something like: >>> //http://msdn.microsoft.com/library/.../directshowstructures.asp >>> [...] >> In theory this sounds good but in practice it will quickly become a >> nightmare. MSDN changes its URL references so that you have to >> continue to search for the documentation. It isn't feasible in the >> case of w32api documentation. > > OK, the URLs may change but the overall structure of the > documentation can't change that often. What if I add the title of the > page in addition to / instead of its URL? Something like: > //Microsoft DirectShow 9.0 - DirectShow Structures To be GNU Coding Standard compliant, and I for one don't know that we should be, you would need to use the /* ... */ method of commenting. However, since our headers would only be used by compilers that support // style comments; I don't think we should be that stringent. > or: > //Microsoft DirectShow 9.0 - DirectShow Structures > //http://msdn.microsoft.com/library/.../directshowstructures.asp > No URLs please. They will become a nightmare to manage. > How often do the URLs change? If they change once in a few years, > then I think it's still useful to add the URLs. URLs that have not > been valid for months may be a sign that the relevant header needs to > be updated... > I'm not sure how often the URLs change but I've found references I've saved to have been removed/changed in a matter of months, not years. Earnie Boyd http://shop.siebunlimited.com |
From: Dimitri Papadopoulos-O. <dim...@ce...> - 2006-05-09 07:20:59
|
Hi, > As the new kid on the admin block, I'm a bit reticent about throwing my > hat into this ring; however, IMO this style of ChangeLog entry, with > asterisks replacing swathes of arbitrary and unspecified symbol names, > should not be tolerated. It is explicitly discouraged by the GNU Coding > Standards, on which our ChangeLog style is modelled; indeed, it could be > argued that it is implicitly forbidden. > > While I appreciate that it may require additional effort to fully and > explicitly itemise every symbol affected by a patch, this complaint from > Brian clearly illustrates the reason why we should do so... It should be enough to have a look at the diff, why duplicate the information? The ChangeLog should *explain* the diff, not *duplicate* it. I'm afraid I'm not not fully aware of the details of the GNU coding standards, so I will read them again. However I won't follow them if they require that I list hundreds of new symbols added to a header file. Dimitri |
From: Keith M. <kei...@us...> - 2006-05-10 19:39:17
|
On Tuesday 09 May 2006 8:20 am, Dimitri Papadopoulos-Orfanos wrote: > > While I appreciate that it may require additional effort to fully and > > explicitly itemise every symbol affected by a patch, this complaint > > from Brian clearly illustrates the reason why we should do so... > > It should be enough to have a look at the diff, why duplicate the > information? The ChangeLog should *explain* the diff, not *duplicate* > it. Rubbish! The ChangeLog should *document* *what* you changed, and *when* you changed it. The diff is merely the *implementation* of your changes; it *never* forms any component of the documentation; indeed, it isn't even retained within the project repository, for future reference. Note that the ChangeLog does *not* document the *why* of your changes; that is also important, but it belongs in comments in your code, or in the Release Notes for the forthcoming release which will incorporate your changes, (which you *should* be amending as you implement those changes). Regards, Keith. |
From: Dimitri Papadopoulos-O. <dim...@ce...> - 2006-05-11 09:42:21
|
Hi, > Rubbish! The ChangeLog should *document* *what* you changed, and *when* > you changed it. The diff is merely the *implementation* of your changes; > it *never* forms any component of the documentation; indeed, it isn't > even retained within the project repository, for future reference. Exactly. Pasting a few hundred macro constants into the ChangeLog is pasting a implementation detail. The exact same information is in the diff. So when I have a hundred lines looking like: ... #define FOOBAR_006 0x0006 ... #define FOOBAR_956 0x0956 ... I just write in the ChangeLog: include/foobar.h (FOOBAR_*): Add define. There's no way I can change that, that would be too much work. Besides I don't understand what you mean by "the diff isn't even retained within the project repository", that's the whole purpose of a revision control system. > Note that the ChangeLog does *not* document the *why* of your changes; > that is also important, but it belongs in comments in your code, or in > the Release Notes for the forthcoming release which will incorporate your > changes, (which you *should* be amending as you implement those changes). In the special case of w32api, the why of the changes cannot go into the code for performance reasons - at least that's what I have been told. Dimitri Papadopoulos |
From: Michael G. <mg...@te...> - 2006-05-11 10:22:14
|
> Exactly. Pasting a few hundred macro constants into the ChangeLog is=20 > pasting a implementation detail. I'm not sure I'm able to follow your argument. > The exact same information is in the diff. Have you ever tried to find a particular change by searching each and every diff of a file with many revisions ? > So when I have a hundred lines looking like:=20 > ... > #define FOOBAR_006 0x0006 > ... > #define FOOBAR_956 0x0956 > ... > I just write in the ChangeLog: > include/foobar.h (FOOBAR_*): Add define. Why not write include/foobar.h: Added define for FOOBAR_006, FOOBAR_007, FOOBAR_008, FOOBAR_009, ... ... FOOBAR_954, FOOBAR_956 I'm pretty sure that could even be generated semiautomatic. Whenever someone looks for changes to any of FOOBAR_* he'll be thankful to find that particular change in the changelog. This becomes more obvious if such changes have been spread across more than a single revision of include/foobar.h > There's no way I can change that, that would be too much work. Come on, e.g. a trivial perlscript could do the job. Or use emacs "regexp search and replace". This is getting silly. > Besides I don't understand what you mean by "the diff isn't even=20 > retained within the project repository", that's the whole purpose of a=20 > revision control system. I think Keith silently implied "in a way that can be searched by your favorite text search tool". E.g. SVN does use a binary format. > In the special case of w32api, the why of the changes cannot go into the= =20 > code for performance reasons - at least that's what I have been told. Why should there be (noticeable) performance degradation ? Do you have the source of this information ? Best, Michael =2D-=20 Vote against SPAM - see http://www.politik-digital.de/spam/ Michael Gerdau email: mg...@te... GPG-keys available on request or at public keyserver |
From: Dimitri Papadopoulos-O. <dim...@ce...> - 2006-05-11 10:30:06
|
Hi, >>The exact same information is in the diff. > > > Have you ever tried to find a particular change by searching each > and every diff of a file with many revisions ? OK, but then that's a problem with CVS, which then needs to be improved. I'm used to Perforce and I have a script that does that. Maybe Perforce behaves better than CVS. Dimitri Papadopoulos |
From: Earnie B. <ea...@us...> - 2006-05-11 10:57:05
|
Quoting Dimitri Papadopoulos-Orfanos <dim...@ce...>: > > In the special case of w32api, the why of the changes cannot go into > the code for performance reasons - at least that's what I have been > told. > Comment remove and space reduction in the w32api headers were accomplished years ago by DJ Delorie IIRC to help with the speed of compiling Cygwin. Perhaps Chris Faylor might want to comment? Earnie Boyd http://shop.siebunlimited.com |
From: Christopher F. <me...@cg...> - 2006-05-12 21:42:36
|
On Thu, May 11, 2006 at 06:57:00AM -0400, Earnie Boyd wrote: >Quoting Dimitri Papadopoulos-Orfanos <dim...@ce...>: >>In the special case of w32api, the why of the changes cannot go into >>the code for performance reasons - at least that's what I have been >>told. > >Comment remove and space reduction in the w32api headers were >accomplished years ago by DJ Delorie IIRC to help with the speed of >compiling Cygwin. Perhaps Chris Faylor might want to comment? You have a better memory than I. I vaguely remember something like this but I don't have any clear recollection of how much of an improvement was seen. cgf |
From: Keith M. <kei...@us...> - 2006-05-10 20:31:45
|
On Tuesday 09 May 2006 8:20 am, Dimitri Papadopoulos-Orfanos wrote: > I'm afraid I'm not not fully aware of the details of the GNU coding > standards, so I will read them again. However I won't follow them if > they require that I list hundreds of new symbols added to a header > file. We don't have to religiously follow the GCS, since we aren't running a GNU project. However, when it comes to itemising added, modified or deleted symbols in a ChangeLog, then *I* don't consider it appropriate to not itemise them individually, even if there are literally hundreds of them. BTW, uttering such truculent refusals to accede to a request from a project administrator is a good way to fast track yourself off the project team. Regards, Keith. |
From: Dimitri Papadopoulos-O. <dim...@ce...> - 2006-05-09 08:15:38
|
Hi again, > As the new kid on the admin block, I'm a bit reticent about throwing my= =20 > hat into this ring; however, IMO this style of ChangeLog entry, with=20 > asterisks replacing swathes of arbitrary and unspecified symbol names,=20 > should not be tolerated. It is explicitly discouraged by the GNU Codin= g=20 > Standards, on which our ChangeLog style is modelled; indeed, it could b= e=20 > argued that it is implicitly forbidden. OK, I've read again the relevant part of the GNU coding standards and=20 indeed: It's important to name the changed function or variable in full. Don't abbreviate function or variable names, and don't combine them. Subsequent maintainers will often search for a function name to find all the change log entries that pertain to it; if you abbreviate the name, they won't find it when they search. Note however that I've just replicated the existing w32api ChangeLog=20 style. Therefore I don't know how much the GNU coding standards apply to=20 w32api. Actually I believe the GNU coding standards do not fit w32api headers=20 well for two reasons: 1) The changes to w32api are very specific. For example what if I add=20 hundreds of line looking like: #define FOOBAR01 0x0001 #define FOOBAR02 0x0002 #define FOOBAR03 0x0003 ... That's very typical of w32api changes and I don't see why I should=20 duplicate the information in ChangeLog - it's already in the diff.=20 Actually I think this would make the ChangeLog unreadable: * include/header.h (FOOBAR01,FOOBAR02,FOOBAR03,FOOBAR04) (FOOBAR05,FOOBAR06,FOOBAR07,FOOBAR08,FOOBAR09,FOOBAR10) ... (FOOBAR96,FOOBAR97,FOOBAR98,FOOBAR99): Define. 2) Until now it was unfortunately impossible to put comments into the=20 code, for the sake of faster parsing of header files. The GNU coding=20 standards specify: There's no need to describe the full purpose of the changes or how they work together. If you think that a change calls for explanation, you're probably right. Please do explain it=97but please put the explanation in comments in the code, where people will see it whenever they see the code. For example, =93New function=94 is enough for the change log when you add a function, because there should be a comment before the function definition to explain what it does. So the explanations have to be put in the ChangeLog. Unless... Since GCC=20 4 supports precompiled headers, would it be possible to add some minimal=20 comments in the header files? I was thinking of a mere link to the=20 relevant documentation, something like: //http://msdn.microsoft.com/library/.../directshowstructures.asp typedef struct _DSFooBar { ... } DSFooBar; typedef struct _DSStruct { ... } DSStruct; In the case where the documentation is wrong or missing, a more detailed=20 explanation could be added. In short, this is a proposal to group macros, functions, structures=20 together according to the MSDN page they belong to, after a one-line=20 comment that is a link to that page. I think this would: 1) help maintain w32api, 2) make clear to everyone contributions should rely on the MSDN. Dimitri Papadopoulos |
From: Earnie B. <ea...@us...> - 2006-05-09 11:25:43
|
Quoting Dimitri Papadopoulos-Orfanos <dim...@ce...>: > So the explanations have to be put in the ChangeLog. Unless... Since > GCC 4 supports precompiled headers, would it be possible to add some > minimal comments in the header files? I was thinking of a mere link > to the relevant documentation, something like: > //http://msdn.microsoft.com/library/.../directshowstructures.asp > typedef struct _DSFooBar { > ... > } DSFooBar; > typedef struct _DSStruct { > ... > } DSStruct; > In the case where the documentation is wrong or missing, a more > detailed explanation could be added. > > In theory this sounds good but in practice it will quickly become a nightmare. MSDN changes its URL references so that you have to continue to search for the documentation. It isn't feasible in the case of w32api documentation. Earnie Boyd http://shop.siebunlimited.com |
From: Keith M. <kei...@us...> - 2006-05-10 20:22:12
|
On Tuesday 09 May 2006 9:15 am, Dimitri Papadopoulos-Orfanos wrote: > OK, I've read again the relevant part of the GNU coding standards and > indeed: > It's important to name the changed function or variable in > full. Don't abbreviate function or variable names, and don't > combine them. Subsequent maintainers will often search for > a function name to find all the change log entries that pertain > to it; if you abbreviate the name, they won't find it when they > search. > Note however that I've just replicated the existing w32api ChangeLog > style. Just because some contributors have been allowed to get away with this unsatisfactory practice in the past, is no reason to perpetuate the bad habit. > Therefore I don't know how much the GNU coding standards apply > to w32api. As Earnie has pointed out, we don't need to enforce them strictly; this doesn't mean that we shouldn't accept their advice, when it is appropriate, and encourages good practice. > Actually I believe the GNU coding standards do not fit w32api headers > well for two reasons: > > 1) The changes to w32api are very specific. For example what if I add > hundreds of line looking like: > #define FOOBAR01 0x0001 > #define FOOBAR02 0x0002 > #define FOOBAR03 0x0003 > ... You itemise each one independently, in the ChangeLog. > That's very typical of w32api changes and I don't see why I should > duplicate the information in ChangeLog - it's already in the diff. As I've already stated in a previous response, the ChangeLog is the permanent record of the change. The diff is transient, and isn't adequately documented, until the relevant and complete record of the changes therein are committed to the ChangeLog. "Already in the diff" doesn't cut it, for the diff is *not* a permanent documentary record. > Actually I think this would make the ChangeLog unreadable: > * include/header.h (FOOBAR01,FOOBAR02,FOOBAR03,FOOBAR04) > (FOOBAR05,FOOBAR06,FOOBAR07,FOOBAR08,FOOBAR09,FOOBAR10) > ... > (FOOBAR96,FOOBAR97,FOOBAR98,FOOBAR99): Define. Sorry, but I disagree; this level of documentation is imperative. > 2) Until now it was unfortunately impossible to put comments into the > code, for the sake of faster parsing of header files. Eh? In 25 years of professional software development, I have never before met any accomplished programmer who would advocate omitting comments from *any* source file. If they cause a performance hit during compilation, then so be it; live with it. In any case, my own experience, with much slower machines than we use today, (I talking 4.7MHz 8088 here), and where any performance gain would therefore be more noticeable, would suggest that any such gain from removal of comments, is negligible. > The GNU coding standards specify: > There's no need to describe the full purpose of the changes > or how they work together. If you think that a change calls > for explanation, you're probably right. Please do explain > it-but please put the explanation in comments in the code, > where people will see it whenever they see the code. For > example, "New function" is enough for the change log when > you add a function, because there should be a comment before > the function definition to explain what it does. Exactly. This is the documentation of *why* you made the change. It *doesn't* belong in the ChangeLog; that's where you document the *what* and the *when* of the change. > So the explanations have to be put in the ChangeLog. Again, eh? Read the above paragraph again; the explanations go in the source -- to suggest that they can't go there is arrant nonsense. > Unless... Since > GCC 4 supports precompiled headers, would it be possible to add some > minimal comments in the header files? I was thinking of a mere link to > the relevant documentation, something like: > //http://msdn.microsoft.com/library/.../directshowstructures.as >p typedef struct _DSFooBar { > ... Now you are confusing source code comments with documentation. You don't need to write the man page into the source file! (However, you could reproduce the `SYNOPSIS' from the man page, as a preamble to each function *implementation*). > In the case where the documentation is wrong or missing, a more > detailed explanation could be added. Yes; you write a *separate* man page. Indeed, even where the function *is* well documented on MSDN, there would be no harm in providing a brief man page, with a `SEE ALSO' section stating that `additional information may be found on MSDN', or words to that effect. (But, note Earnie's comments on the perils of quoting URLs in such documentation). Regards, Keith. |
From: Dimitri Papadopoulos-O. <dim...@ce...> - 2006-05-11 09:46:24
|
Hi, > As Earnie has pointed out, we don't need to enforce them strictly; this > doesn't mean that we shouldn't accept their advice, when it is > appropriate, and encourages good practice. > [...] >>1) The changes to w32api are very specific. For example what if I add >>hundreds of line looking like: >> #define FOOBAR01 0x0001 >> #define FOOBAR02 0x0002 >> #define FOOBAR03 0x0003 >> ... > > > You itemise each one independently, in the ChangeLog. I'm sorry, but since we don't need to enforce the GNU standards strictly, I won't do that for a hundred items added to a header file. If I have to, I just won't commit anymore. Dimitri Papadopoulos |
From: Dimitri Papadopoulos-O. <dim...@ce...> - 2006-05-09 08:23:42
|
Hi, Another part of the standard that seems not to be followed in w32api: Calculate column numbers assuming that space and all ASCII printing characters have equal width, and assuming tab stops every 8 columns. ^^^^^^^^^^^^^^^^^^^^^^^^^ Dimitri |