Trailing white space and other pedantry.
From: Max TenEyck Woodbury<max(a)mtew.isa-geek.net> I have been working on the documentation extraction problem and code analysis problem. I have made some progress with the program I am writing to do this. It reads the same files as c2man.pl does, but does a more detailed parse of the code. In fact, it even reads and processes all the include files. It is far from complete at this time, but it does produce interesting warnings. One of the warnings reports lines with trailing while space. It has turned up quite a few places where this occurs. If I understand the preferred style, there should not be trailing white space. The question I have is what should I do with them. I can either fix the files or turn that particular warning off. So far, I have been fixing the files on a local copy of the repository. Should I turn the fixes into patches and submit them, or just keep them to myself? Another problem the program warns about is incompatible macro redefinitions. In a few places the differences are just white space, and I have made local fixes. In other places, the differences are more substantial and there are two basic options available. The more difficult solution is to change the definitions in wine to make them compatible with the rest of the system. This could cause problems for people with different system configurations. The other solution is to simply #undef the macros before redefining them. Should I turn the simple fixes into patches and submit them? Should I submit the other problems as bug reports? Max
Am 16.05.2011 18:34, schrieb max(a)mtew.isa-geek.net:
From: Max TenEyck Woodbury<max(a)mtew.isa-geek.net>
I have been working on the documentation extraction problem and code analysis problem.
I have made some progress with the program I am writing to do this. It reads the same files as c2man.pl does, but does a more detailed parse of the code. In fact, it even reads and processes all the include files. It is far from complete at this time, but it does produce interesting warnings.
One of the warnings reports lines with trailing while space. It has turned up quite a few places where this occurs. If I understand the preferred style, there should not be trailing white space. The question I have is what should I do with them. I can either fix the files or turn that particular warning off. So far, I have been fixing the files on a local copy of the repository.
Should I turn the fixes into patches and submit them, or just keep them to myself?
No, whitespace only changes will not be accepted. Your are right about the preferred style, but most likely the regarding code is a bit old. new code should never have trailing whitespace. The best reason is "git blame" to see who wrote the code, i think that makes it clear. -- Best Regards, André Hentschel
On 05/16/2011 01:00 PM, André Hentschel wrote:
Am 16.05.2011 18:34, schrieb max(a)mtew.isa-geek.net:
From: Max TenEyck Woodbury<max(a)mtew.isa-geek.net>
I have been working on the documentation extraction problem and code analysis problem.
I have made some progress with the program I am writing to do this. It reads the same files as c2man.pl does, but does a more detailed parse of the code. In fact, it even reads and processes all the include files. It is far from complete at this time, but it does produce interesting warnings.
One of the warnings reports lines with trailing while space. It has turned up quite a few places where this occurs. If I understand the preferred style, there should not be trailing white space. The question I have is what should I do with them. I can either fix the files or turn that particular warning off. So far, I have been fixing the files on a local copy of the repository.
Should I turn the fixes into patches and submit them, or just keep them to myself?
No, white space only changes will not be accepted. Your are right about the preferred style, but most likely the regarding code is a bit old. new code should never have trailing white space. The best reason is "git blame" to see who wrote the code, i think that makes it clear.
I understand that white space only patches will not be applied. Will they be applied if they accompany other corrections? Also, please address the other questions. Should I submit patches against simple errors in macro definition formatting? There are some places where my program catches mismatches that SOME other compilers might ignore. There are also some places in the wine headers where macros are redefined differently from the headers provided by the system or compilers I have installed. There are also cases where the macros are defined differently by different wine headers. Because these problems may depend on my system configuration, I believe it might not be possible to simply patch these definitions match the configuration I have. The problems CAN be fixed simply by adding #undef before each redefinition, but I think a review of these changes might be in order. They seem to occur in batches, with none in most headers and multiple messes in others. Should I submit each correction as a separate patch, separate patches accompanying separate bug reports, combined patches for a given header or combined patch and bug report? Max
On May 16, 2011, at 7:42 PM, max wrote:
On 05/16/2011 01:00 PM, André Hentschel wrote:
Am 16.05.2011 18:34, schrieb max(a)mtew.isa-geek.net:
From: Max TenEyck Woodbury<max(a)mtew.isa-geek.net>
I have been working on the documentation extraction problem and code analysis problem.
I have made some progress with the program I am writing to do this. It reads the same files as c2man.pl does, but does a more detailed parse of the code. In fact, it even reads and processes all the include files. It is far from complete at this time, but it does produce interesting warnings.
One of the warnings reports lines with trailing while space. It has turned up quite a few places where this occurs. If I understand the preferred style, there should not be trailing white space. The question I have is what should I do with them. I can either fix the files or turn that particular warning off. So far, I have been fixing the files on a local copy of the repository.
Should I turn the fixes into patches and submit them, or just keep them to myself?
No, white space only changes will not be accepted. Your are right about the preferred style, but most likely the regarding code is a bit old. new code should never have trailing white space. The best reason is "git blame" to see who wrote the code, i think that makes it clear.
I understand that white space only patches will not be applied. Will they be applied if they accompany other corrections?
If one would otherwise have modified a line in the course of doing development, then one can and should fix an issue like trailing whitespace on that line. If the line would not have been changed except for the whitespace issue, then it shouldn't be changed.
Also, please address the other questions.
Well, he might not have known the answer to or had an opinion about the other questions.
Should I submit patches against simple errors in macro definition formatting? There are some places where my program catches mismatches that SOME other compilers might ignore.
There are also some places in the wine headers where macros are redefined differently from the headers provided by the system or compilers I have installed. There are also cases where the macros are defined differently by different wine headers. Because these problems may depend on my system configuration, I believe it might not be possible to simply patch these definitions match the configuration I have. The problems CAN be fixed simply by adding #undef before each redefinition, but I think a review of these changes might be in order. They seem to occur in batches, with none in most headers and multiple messes in others.
Am I understanding correctly that these are whitespace difference again? Maybe you could provide an example of what you mean. To my knowledge the mismatches you have identified have never actually caused problems. Until they do, I expect that fixes for them won't be accepted. Also, when they do, that will probably help guide which of your proposed solutions is preferred.
Should I submit each correction as a separate patch, separate patches accompanying separate bug reports, combined patches for a given header or combined patch and bug report?
Hard to say without an illustration of what you're talking about. Regards, Ken
On 05/16/2011 09:39 PM, Ken Thomases wrote:
On May 16, 2011, at 7:42 PM, max wrote:
On 05/16/2011 01:00 PM, André Hentschel wrote:
I understand that white space only patches will not be applied. Will they be applied if they accompany other corrections? If one would otherwise have modified a line in the course of doing development, then one can and should fix an issue like trailing whitespace on that line. If the line would not have been changed except for the whitespace issue, then it shouldn't be changed.
Also, please address the other questions. Well, he might not have known the answer to or had an opinion about the other questions.
Should I submit patches against simple errors in macro definition formatting? There are some places where my program catches mismatches that SOME other compilers might ignore.
There are also some places in the wine headers where macros are redefined differently from the headers provided by the system or compilers I have installed. There are also cases where the macros are defined differently by different wine headers. Because these problems may depend on my system configuration, I believe it might not be possible to simply patch these definitions match the configuration I have. The problems CAN be fixed simply by adding #undef before each redefinition, but I think a review of these changes might be in order. They seem to occur in batches, with none in most headers and multiple messes in others. Am I understanding correctly that these are whitespace difference again? Maybe you could provide an example of what you mean.
To my knowledge the mismatches you have identified have never actually caused problems. Until they do, I expect that fixes for them won't be accepted. Also, when they do, that will probably help guide which of your proposed solutions is preferred. FYI: Scanning (C) source file 'dlls/atl/atl_ax.c'. --Warning: Trailing white space at dlls/atl/atl_ax.c:991. --Warning: Trailing white space at dlls/atl/atl_ax.c:1005. --Warning: Trailing white space at dlls/atl/atl_ax.c:1027. --Warning: Trailing white space at dlls/atl/atl_ax.c:1042. --Warning: Trailing white space at dlls/atl/atl_ax.c:1131. --Warning: Trailing white space at dlls/atl/atl_ax.c:1141. --Warning: Trailing white space at dlls/atl/atl_ax.c:1245. Scanning (C) source file 'dlls/atl/registrar.c'. --Warning: Trailing white space at dlls/atl/registrar.c:206. --Warning: Trailing white space at dlls/atl/registrar.c:213. --Warning: Trailing white space at dlls/atl/registrar.c:243. --Warning: Trailing white space at dlls/atl/registrar.c:380. --Warning: Trailing white space at dlls/atl/registrar.c:525. --Warning: Trailing white space at dlls/atl/registrar.c:597.
Should I submit each correction as a separate patch, separate patches accompanying separate bug reports, combined patches for a given header or combined patch and bug report? Hard to say without an illustration of what you're talking about. In the simple cases, they would produce compiler warnings, not functional issues. In other cases, there are differences that I expect do not show up because the wine project does not use the macro, but could cause application programmers grief. For example:
Scanning (C) source file 'dlls/advapi32/service.c'. ==Error: Attempt to change the definition of macro '__attribute__' at line 88 of include/wine/exception.h ignored. Originally defined at /usr/include/sys/cdefs.h:203 Old:' ( xyz ) ' New:' ( x ) ' Scanning (C) source file 'dlls/avicap32/avicap32_main.c'. ==Error: Attempt to change the definition of macro 'IOC_OUT' at line 697 of include/winsock.h ignored. Originally defined at /usr/include/asm-generic/ioctl.h:91 Old:' ( _IOC_READ << _IOC_DIRSHIFT ) ' New:' 1073741824 ' ==Error: Attempt to change the definition of macro 'IOC_IN' at line 698 of include/winsock.h ignored. Originally defined at /usr/include/asm-generic/ioctl.h:90 Old:' ( _IOC_WRITE << _IOC_DIRSHIFT ) ' New:' 2147483648 ' ==Error: Attempt to change the definition of macro 'IOC_INOUT' at line 699 of include/winsock.h ignored. Originally defined at /usr/include/asm-generic/ioctl.h:92 Old:' ( ( _IOC_WRITE | _IOC_READ ) << _IOC_DIRSHIFT ) ' New:' ( IOC_IN | IOC_OUT ) ' ==Error: Attempt to change the definition of macro '_IO' at line 701 of include/winsock.h ignored. Originally defined at /usr/include/asm-generic/ioctl.h:74 Old:' ( type , nr ) _IOC ( _IOC_NONE , ( type ) , ( nr ) , 0 ) ' New:' ( x , y ) ( IOC_VOID | ( ( x ) << 8 ) | ( y ) ) ' ==Error: Attempt to change the definition of macro '_IOR' at line 702 of include/winsock.h ignored. Originally defined at /usr/include/asm-generic/ioctl.h:75 Old:' ( type , nr , size ) _IOC ( _IOC_READ , ( type ) , ( nr ) , ( _IOC_TYPECHECK ( size ) ) ) ' New:' ( x , y , t ) ( IOC_OUT | ( ( ( UINT ) sizeof ( t ) & IOCPARM_MASK ) << 16 ) | ( ( x ) << 8 ) | ( y ) ) ' ==Error: Attempt to change the definition of macro '_IOW' at line 703 of include/winsock.h ignored. Originally defined at /usr/include/asm-generic/ioctl.h:76 Old:' ( type , nr , size ) _IOC ( _IOC_WRITE , ( type ) , ( nr ) , ( _IOC_TYPECHECK ( size ) ) ) ' New:' ( x , y , t ) ( IOC_IN | ( ( ( UINT ) sizeof ( t ) & IOCPARM_MASK ) << 16 ) | ( ( x ) << 8 ) | ( y ) ) ' Max
participants (4)
-
André Hentschel -
Ken Thomases -
max -
max＠mtew.isa-geek.net