WineHQ

Code Hygiene: Difference between revisions

(Begin organizing Code Hygiene page)
 
No edit summary
Line 10: Line 10:
This shouldn't be nearly as much of a problem now (it might be resolved entirely), but any changes should respect the separation so be sure that your submission:
This shouldn't be nearly as much of a problem now (it might be resolved entirely), but any changes should respect the separation so be sure that your submission:
* Places code for 16-bit support in its own file in the appropriate 16-bit dll
* Places code for 16-bit support in its own file in the appropriate 16-bit dll
* Don't call 16-bit DLLs from a 32-bit one (with <tt>LoadLibrary16</tt> or <tt>GetModuleHandle16</tt>)
* Doesn't call 16-bit DLLs from a 32-bit one (with <tt>LoadLibrary16</tt> or <tt>GetModuleHandle16</tt>)
* Don't use a 16-bit include file for any 32-bit code
* Doesn't use a 16-bit include file for any 32-bit code
* Don't implement a 16-bit function in a file for 32-bit code
* Doesn't implement a 16-bit function in a file for 32-bit code


{{Info}} If you come across a situation where you believe making an exception to these rules is necessary, you can definitely discuss it on the wine developer's mailing list.
 
{{Info}} If you come across what you believe is a necessary exception, you can always discuss it on the wine developer's mailing list.


== Set Read-Only Data to const ==
== Set Read-Only Data to const ==
This task actually isn't too difficult. After compiling whatever files you're working on, you can use the <tt>objdump -x</tt> tool on the resulting object files, then cross-reference the ''.data'' and ''.rodata'' segments.
This task actually isn't too difficult. After compiling whatever files you're working on, you can use <tt>objdump -x</tt> on the resulting object files, then cross-reference the ''.data'' and ''.rodata'' segments from the dump. Any variables in ''.data'' that shouldn't be written to can be set as <tt>const</tt> in your source, which should place them in ''.rodata'' once you rebuild the files. Of course, be sure to fix any new compile warnings your changes generate too.
 
{{Warning}} After you've made your changes, rebuild and actually '''run''' your modified version of wine to test. It's important you do some real-world testing because a variable incorrectly set as const can often cause segfaults.
 
== Duplicate Code ==
When working on a problem that's similar to one addressed elsewhere, "cut-and-paste" coding can be a kludgy but very effective way to prototype. The goal should always be to minimize duplication in your patches though, bundling common code into distinct modules whenever possible. That said, you can have situations where the current structure of the source or the build process would make this kind of refactoring a significant project.


Any variables in ''.data'' that shouldn't be written to can be set as const in your source, which should place them in ''.rodata'' once you rebuild them. Of course, be sure to fix any new compile warnings your changes generate too.
In developing wine, we've come across a few situations like this, where refactoring still isn't expedient. So in the meanwhile, we try to at least keep the code in sync. Here is a list of current "cut-and-paste" relationships in the wine source tree:


{{Warning}} After you've made your changes, rebuild and actually '''run''' your modified version of wine to test. It's important you do some real-world testing because a variable incorrectly set as const can often cause segfaults.
{| class="wikitable"
! Purpose !! File A !! File B !! Current Status
|-
| Demangle VC++ symbols into C function prototypes || [http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/msvcrt/undname.c dlls/msvcrt/undname.c] || [http://source.winehq.org/git/wine.git/blob/HEAD:/tools/winedump/msmangle.c tools/winedump.msmangle.c] || Memory allocation needs to be rewritten to allow for a complete sync
|-
|| Read dwarf2 info from ELF modules || [http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/dbghelp/dwarf.c dlls/dbghelp/dwarf.c] || [http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/ntdll/signal_x86_64.c dlls/ntdll/signal_x86_64.c] || 100% synced since 27 Mar 2010
|-
|| Use the minidump method for debug info || [http://source.winehq.org/git/wine.git/blob/HEAD:/tools/winedump/minidump.c tools/winedump/minidump.c] || [http://source.winehq.org/git/wine.git/blob/HEAD:/programs/winedbg/tgt_minidump.c programs/winedbg/tgt_minidump.c] || 100% synced since 29 Aug 2015
|}
 
Remember that these duplications are seen as necessary evils, but if you sincerely believe something similar is the best solution for your problem at the moment, definitely discuss it with the other developers on the wine-devel mailing list. Also, if you do get your patch submitted, please add an entry for your duplication to the above table.


== See Also ==
== See Also ==

Revision as of 19:35, 11 March 2016

Work in progress: This page is currently undergoing extensive revision. External links to specific parts of this page may now be broken due to the section having been edited, moved, merged with another section, or removed altogether. Consult the table of contents to find the section you are looking for. There may be related discussion on the talk page.

In the past, submitting code to wine had a reputation for being difficult. We're trying several changes to make it easier for people to contribute code, but one thing we don't want to let go of are high standards for patches. Although we can't list all of the issues that might come up, here are a few that drew a lot of attention in the past.

Many of these could theoretically be checked automatically, and there are already basic scripts floating around for some of them. It hasn't been discussed yet, but having our testbot check for these down the road could be a worthwhile project.

16-bit Separation

Wine supports old 16-bit executables, but since many users might not be interested in this feature, we have a build option to disable it: --disable-win16. However, especially in its early versions, wine had a significant amount of overlap between its 16 and 32-bit functions.

This shouldn't be nearly as much of a problem now (it might be resolved entirely), but any changes should respect the separation so be sure that your submission:

  • Places code for 16-bit support in its own file in the appropriate 16-bit dll
  • Doesn't call 16-bit DLLs from a 32-bit one (with LoadLibrary16 or GetModuleHandle16)
  • Doesn't use a 16-bit include file for any 32-bit code
  • Doesn't implement a 16-bit function in a file for 32-bit code


If you come across what you believe is a necessary exception, you can always discuss it on the wine developer's mailing list.

Set Read-Only Data to const

This task actually isn't too difficult. After compiling whatever files you're working on, you can use objdump -x on the resulting object files, then cross-reference the .data and .rodata segments from the dump. Any variables in .data that shouldn't be written to can be set as const in your source, which should place them in .rodata once you rebuild the files. Of course, be sure to fix any new compile warnings your changes generate too.

After you've made your changes, rebuild and actually run your modified version of wine to test. It's important you do some real-world testing because a variable incorrectly set as const can often cause segfaults.

Duplicate Code

When working on a problem that's similar to one addressed elsewhere, "cut-and-paste" coding can be a kludgy but very effective way to prototype. The goal should always be to minimize duplication in your patches though, bundling common code into distinct modules whenever possible. That said, you can have situations where the current structure of the source or the build process would make this kind of refactoring a significant project.

In developing wine, we've come across a few situations like this, where refactoring still isn't expedient. So in the meanwhile, we try to at least keep the code in sync. Here is a list of current "cut-and-paste" relationships in the wine source tree:

Purpose File A File B Current Status
Demangle VC++ symbols into C function prototypes dlls/msvcrt/undname.c tools/winedump.msmangle.c Memory allocation needs to be rewritten to allow for a complete sync
Read dwarf2 info from ELF modules dlls/dbghelp/dwarf.c dlls/ntdll/signal_x86_64.c 100% synced since 27 Mar 2010
Use the minidump method for debug info tools/winedump/minidump.c programs/winedbg/tgt_minidump.c 100% synced since 29 Aug 2015

Remember that these duplications are seen as necessary evils, but if you sincerely believe something similar is the best solution for your problem at the moment, definitely discuss it with the other developers on the wine-devel mailing list. Also, if you do get your patch submitted, please add an entry for your duplication to the above table.

See Also

  • For an introduction to COM, Jeff Glatt's "COM in Plain C" articles are very good
This page was last edited on 11 March 2016, at 19:35.