GNOME Bugzilla – Bug 539297
Make include guards unique
Last modified: 2013-09-18 16:51:52 UTC
Some include guards contain only a short object name. I find name clashes likely if the header files are intended to be reuseable by other software projects. Examples: http://svn.gnome.org/viewvc/gparted/trunk/include/Operation.h?revision=810&view=markup http://svn.gnome.org/viewvc/gparted/trunk/include/Partition.h?revision=710&view=markup How do you think about to make these names longer? http://en.wikipedia.org/wiki/Include_guard#Difficulties
Created attachment 173904 [details] [review] update suggestion Would you like to integrate any of the appended changes into your source code repository?
Thank you Markus for bringing this to my attention and attaching a proposed patch. You have certainly found some inconsistencies with the include guards in the GParted code. As you point out: Operation.h uses OPERATION Partion.h uses PARTITION whereas other areas use: DMRaid.h uses DMRAID_H_ In the patch it appears that the include guard is built by prefixing GPARTED_ in front of the filename, and suffixing an underscore with a uniquely generated hex string at the end of the file name. Operation.h GPARTED_OPERATION_H_B92ECD105630419EA71B9CE976FA56E2 How did you generate the unique hex string? Are other projects using similarly named include guards? If so, would you be able to point me to one of these projects? My initial thoughts are that prefixing GPARTED_ in front of the file name should be sufficient to make the include guard unique, in case other projects would like to include GParted code. Is there a situation you can think of in which the extra uniquely generated hex string would also be required?
(In reply to comment #2) > How did you generate the unique hex string? I reused the function "uuid_generate_random" in a small program which converts the returned value into a hexadecimal number that I appended from its display in a terminal window into the affected header files by "copy and paste". http://linux.die.net/man/3/uuid_generate > Are other projects using similarly named include guards? simon listens https://sourceforge.net/tracker/?func=detail&atid=935103&aid=2852162&group_id=190872 http://speech2text.git.sourceforge.net/git/gitweb.cgi?p=speech2text/speech2text;a=commit;h=45a1e3e7bfa7ed4f2de519c0af35d93a01fb31dd http://speech2text.git.sourceforge.net/git/gitweb.cgi?p=speech2text/speech2text;a=commit;h=08049b55d7b30f92dbda9da42cf999d97b35b028 Ceph http://tracker.newdream.net/issues/98#note-9 http://tracker.newdream.net/projects/ceph/repository/revisions/f4b9d9d847627e2445cd8bd80c6b893cd9efcd30 > Is there a situation you can think of in which the extra uniquely generated hex > string would also be required? Would you like to reuse universally unique identifiers to make include guards safer? Which names should become an official part of your application programming interface?
By the way: Would you like to try out the tool "Wave"? http://boost.org/doc/libs/1_44_0/libs/wave/doc/preface.html
Hi Markus, I've just come across this bug report. GParted code is a standalone application and not a library so we only need to ensure that #include guards are unique between it and all the library header files it uses (C & C++ stand, Glib(mm) and Gtk(mm) libraries), not all code everywhere in the world. As such using a guard name of APPLICATION_MODULE_H is adequate and adding a UUID is not warranted. Would you be interested in updating your to apply to the current code base and using this model? #ifndef GPARTED_MODULE_H #define GPARTED_MODULE_H ... #endif /* GPARTED_MODULE_H */ Thanks, Mike
Created attachment 246062 [details] [review] Unique include guards (v1) Hi Curtis, I've coded a fix so that we can close this bug report. I've also added a cleanup removing unused read-only capability of the internal block copying. Confirmed that the readonly parameter is now always false with debugging of the previous code. Tested all the paths to copy_filesystem(), which calls copy_blocks(). Before for debugging and after to test the patch. 1) Copy partition 2) Move partition without overlap 3) Move partition with overlap 4) Move partition with overlap and cancel Refs: GParted_Core::copy(), move_filesystem() & rollback_transaction(). Thanks, Mike
Created attachment 246079 [details] [review] Unique include guards (v2) Doh! Here's what I intended to submit. Only difference is to initialise benchmark_blocksize and N as two separate commands rather than using comma separated list of variable initialisations. 1536c1536,1537 < + Byte_Value benchmark_blocksize = (1 * MEBIBYTE), N = (16 * MEBIBYTE) ; --- > + Byte_Value benchmark_blocksize = (1 * MEBIBYTE) ; > + Byte_Value N = (16 * MEBIBYTE) ; The original code with a ternary operator and a comma really confused me to begin with. Thanks, Mike
Hi Mike, Thank you Mike for the updated patch set. I agree with your logic in comment #5. The patch set in comment #37 has been tested on Debian 6, ubuntu 10.04, and kubuntu 12.04. My tests of copies and moves including cancel actions performed as expected. All tests passed including git-test-sequence on Debian 6 and ubuntu 10.04. This patch set has been committed to the Gnome repository for inclusion in the next release of GParted. The relevant git commits can be viewed at the following links: Make include guards unique (#539297) https://git.gnome.org/browse/gparted/commit/?id=2b51d8714739d5ed55c9e08b70ba99e4b14c19af Remove unused read-only functionality from internal block copy https://git.gnome.org/browse/gparted/commit/?id=aed1fb58cf45ff26439c0906c3609ec888d251d2
The patches to address this report have been included in GParted 0.16.2 released on September 18, 2013.