LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ingo Molnar <mingo@elte.hu>
To: Andi Kleen <ak@suse.de>
Cc: ying.huang@intel.com, tglx@linutronix.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] [6/8] Account overlapped mappings in end_pfn_map
Date: Mon, 11 Feb 2008 14:55:35 +0100	[thread overview]
Message-ID: <20080211135535.GA14502@elte.hu> (raw)
In-Reply-To: <200802111427.16288.ak@suse.de>


* Andi Kleen <ak@suse.de> wrote:

> > your patch is also a bit unclean:
> 
> Ok patch with hungarized variables appended.

My comments have nothing to do with "hungarized variables". You used 
clearly unclean and ambigious coding constructs like:

+static unsigned long __meminit
 phys_pmd_update(pud_t *pud, unsigned long address, unsigned long end)
 {
+       unsigned long r;
        pmd_t *pmd = pmd_offset(pud, 0);
        spin_lock(&init_mm.page_table_lock);
-       phys_pmd_init(pmd, address, end);
+       r = phys_pmd_init(pmd, address, end);
        spin_unlock(&init_mm.page_table_lock);
        __flush_tlb_all();
+       return r;
 }

the "r" name is totally unintuitive and the reader of the code is given 
absolutely no idea what happens here.

The cleanliness rules about descriptive variable names are obvious, 
necessary and well respected throughout the kernel, by all other kernel 
contributors i know. For years you were allowed to merge such patches. 
You should really (re-)learn and accept the rules that all other kernel 
contributors have been following for years.

	Ingo

  reply	other threads:[~2008-02-11 13:55 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-11  9:34 [PATCH] [0/8] Various kernel mapping bug fixes Andi Kleen
2008-02-11  9:34 ` [PATCH] [1/8] CPA: Fix gbpages support in try_preserve_lage_page Andi Kleen
2008-02-11  9:45   ` Thomas Gleixner
2008-02-11 10:12     ` Ingo Molnar
2008-02-11 11:01       ` Andi Kleen
2008-02-11  9:34 ` [PATCH] [2/8] CPA: Flush the caches when setting pages not present Andi Kleen
2008-02-11 11:00   ` Ingo Molnar
2008-02-11 12:26     ` Andi Kleen
2008-02-11  9:34 ` [PATCH] [3/8] CPA: Test the correct mapping alias on x86-64 Andi Kleen
2008-02-11 11:49   ` Ingo Molnar
2008-02-11  9:34 ` [PATCH] [4/8] CPA: Fix set_memory_x for ioremap Andi Kleen
2008-02-11 12:27   ` Ingo Molnar
2008-02-11 12:45     ` Andi Kleen
2008-02-11  9:34 ` [PATCH] [5/8] Fix logic error in 64bit memory hotadd Andi Kleen
2008-02-11 12:46   ` Ingo Molnar
2008-02-11 13:05     ` Andi Kleen
2008-02-11 13:33       ` Ingo Molnar
2008-02-11 13:44         ` Andi Kleen
2008-02-12 10:35       ` Yasunori Goto
2008-02-12 11:20         ` Andi Kleen
2008-02-11  9:34 ` [PATCH] [6/8] Account overlapped mappings in end_pfn_map Andi Kleen
2008-02-11 13:08   ` Ingo Molnar
2008-02-11 13:27     ` Andi Kleen
2008-02-11 13:55       ` Ingo Molnar [this message]
2008-02-11 14:16       ` Peter Zijlstra
2008-02-11 14:24         ` Andi Kleen
2008-02-11 14:41           ` Sam Ravnborg
2008-02-11 15:12   ` Arjan van de Ven
2008-02-11  9:34 ` [PATCH] [7/8] Implement true end_pfn_mapped for 32bit Andi Kleen
2008-02-12 19:39   ` Thomas Gleixner
2008-02-12 19:49     ` Andi Kleen
2008-02-12 20:25       ` Thomas Gleixner
2008-02-11  9:34 ` [PATCH] [8/8] RFC: Fix some EFI problems Andi Kleen
2008-02-12 20:04   ` Thomas Gleixner
2008-02-12 20:23     ` Andi Kleen
2008-02-12 20:48       ` Thomas Gleixner
2008-02-13 11:05         ` Andi Kleen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080211135535.GA14502@elte.hu \
    --to=mingo@elte.hu \
    --cc=ak@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=ying.huang@intel.com \
    --subject='Re: [PATCH] [6/8] Account overlapped mappings in end_pfn_map' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).