From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30950 invoked by alias); 2 Dec 2007 10:05:55 -0000 Received: (qmail 30942 invoked by uid 22791); 2 Dec 2007 10:05:54 -0000 X-Spam-Check-By: sourceware.org Received: from anyanka.rfc1149.net (HELO mail2.rfc1149.net) (81.56.47.149) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sun, 02 Dec 2007 10:05:50 +0000 Received: from localhost (localhost [127.0.0.1]) by mail2.rfc1149.net (Postfix) with ESMTP id 7ED4FC405C; Sun, 2 Dec 2007 11:05:39 +0100 (CET) Received: from mail2.rfc1149.net ([127.0.0.1]) by localhost (localhost [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Zd+mdV5aQY7h; Sun, 2 Dec 2007 11:05:39 +0100 (CET) Received: by mail2.rfc1149.net (Postfix, from userid 1000) id 57AFFC405E; Sun, 2 Dec 2007 11:05:39 +0100 (CET) To: gcc@gcc.gnu.org Subject: Rant about ChangeLog entries and commit messages Date: Sun, 02 Dec 2007 10:05:00 -0000 User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii From: Samuel Tardieu Content-Transfer-Encoding: 8bit X-WWW: http://www.rfc1149.net/sam X-Jabber: (see http://www.jabber.org/) X-OpenPGP-Fingerprint: 79C0 AE3C CEA8 F17B 0EF1 45A5 F133 2241 1B80 ADE6 (see http://www.gnupg.org/) Message-Id: <2007-12-02-11-05-39+trackit+sam@rfc1149.net> Mailing-List: contact gcc-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-owner@gcc.gnu.org X-SW-Source: 2007-12/txt/msg00016.txt.bz2 As a recent committer to GCC, I am always surprised to see the content of ChangeLog entries and commit messages. I tend to find GCC ChangeLog entries useless. For example, the more recent ChangeLog entry in gcc/ChangeLog is: | 2007-11-30 Jan Hubicka | | * ggc-common.c (dump_ggc_loc_statistics): Reset ggc_force_collect | flag. How could a newcomer guess why the gcc_force_collect flag needs to be reset? Jan posted a useful explanation on gcc-patches, but finding it by searching the mailing-list is not practical and it is not coupled with the checkin. Let's look at the corresponding svn log message, which can be found with "svn blame" if a particular line needs to be pinpointed: | r130560 | hubicka | 2007-12-01 22:06:31 +0100 (Sat, 01 Dec 2007) | 4 lines | | * ggc-common.c (dump_ggc_loc_statistics): Reset ggc_force_collect | flag. Ok, same information is mirrored here, not useful. Let's look at the change itself then: | Index: gcc/ChangeLog | =================================================================== | --- gcc/ChangeLog (revision 130559) | +++ gcc/ChangeLog (revision 130560) | @@ -1,3 +1,8 @@ | +2007-11-30 Jan Hubicka | + | + * ggc-common.c (dump_ggc_loc_statistics): Reset ggc_force_collect | + flag. | + | 2007-11-30 Seongbae Park | | PR rtl-optimization/34171 | Index: gcc/ggc-common.c | =================================================================== | --- gcc/ggc-common.c (revision 130559) | +++ gcc/ggc-common.c (revision 130560) | @@ -1018,5 +1018,6 @@ | fprintf (stderr, "%-48s %10s %10s %10s %10s %10s\n", | "source location", "Garbage", "Freed", "Leak", "Overhead", "Times"); | fprintf (stderr, "-------------------------------------------------------\n") | ; | + ggc_force_collect = false; | #endif | } Ok, still the same information because of the ChangeLog diff, and we can see that the change shows that... gcc_force_collect is reset. Wow! Now, compare that with Jan's message on the list: | pre-ipa-mem-reports force GGC to be done at each invokation in order to | collect data on live memory references, but forgets to reset the flag | after done. This means that compiler continues GGCcollecting and works | slowly. This *is* the information I would expect to be present somewhere in GCC history. A clear and detailed information on why the change was necessary. Sure, in some case the checkin references a PR, but the PR often contains information of what didn't work before the change and the same information which is already repeated three times (ChangeLog, svn log and svn diff). Compare this to a typical commit in the Linux kernel: | commit b1812582ba94b5f377d5d3cec7646cc17d84e733 | Author: Joachim Fenkes | Date: Fri Nov 30 16:19:41 2007 -0800 | | IB/ehca: Fix static rate if path faster than link | | The formula would yield -1 if the path is faster than the link, which | is wrong in a bad way (max throttling). Clamp to 0, which is the | correct value. | | Signed-off-by: Joachim Fenkes | Signed-off-by: Roland Dreier | | diff --git a/drivers/infiniband/hw/ehca/ehca_av.c b/drivers/infiniband/hw/ehca/ehca_av.c | index 453eb99..f7782c8 100644 | --- a/drivers/infiniband/hw/ehca/ehca_av.c | +++ b/drivers/infiniband/hw/ehca/ehca_av.c | @@ -76,8 +76,12 @@ int ehca_calc_ipd(struct ehca_shca *shca, int port, | | link = ib_width_enum_to_int(pa.active_width) * pa.active_speed; | | - /* IPD = round((link / path) - 1) */ | - *ipd = ((link + (path >> 1)) / path) - 1; | + if (path >= link) | + /* no need to throttle if path faster than link */ | + *ipd = 0; | + else | + /* IPD = round((link / path) - 1) */ | + *ipd = ((link + (path >> 1)) / path) - 1; | | return 0; | } What do we have here? A one-line high-level description identifying what has been done, a synthetic analysis of the problem cause and its solution, and the audit trail with the Signed-off-by lines (which in the Linux case is more important than in GCC as copyrights are not assigned to one entity). Linux doesn't use ChangeLog, but its history is much more useful to developers and casual observers than GCC one. And it could be done for GCC (with SVN) as well. Maybe we should consider dropping ChangeLogs and using better checkin messages. Sam