xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@suse.com>
To: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Keir Fraser <keir@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	mpohlack@amazon.de, Ross Lagerwall <ross.lagerwall@citrix.com>,
	sasha.levin@oracle.com,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v4 08/34] vmap: Make the while loop less fishy.
Date: Mon, 21 Mar 2016 09:05:23 -0600	[thread overview]
Message-ID: <56F01BC302000078000DEE58@prv-mh.provo.novell.com> (raw)
In-Reply-To: <CAFLBxZaqpdw=OYm-5ig8L159rV8UMJ-KZdJUpG6j+5XMC3sP5g@mail.gmail.com>

>>> On 21.03.16 at 15:22, <George.Dunlap@eu.citrix.com> wrote:
> Or to take a different tack: I understand that you don't think there's
> no particular benefit to adding a comment in cases like this; could
> you explain to me why you think it would have a significant cost?

There's no significant cost here. Yet I do think that commenting the
obvious is not really helpful (or else we end up with more comments
than there is actual code; some may consider this a good thing, but
I'm of the opinion that this would only serve obfuscating the code).
Plus - as said in various other contexts - I'm in favor of consistency,
i.e. I would think that if these constructs warrant a comment, all
of them should get one. The fall through example you gave for
switch statements is actually a good one: There the comments
silence Coverity. If comments here had the same effect, I think I'd
have no reservations against adding such, yet I don't think they
do.

In the end it boils down to: I'm not going to ack any such change
myself, but I'm also not going to stand in the way of other
maintainers ack-ing such comments getting added. What I would
object to though is calling such constructs "fishy" in commit titles,
commit messages, or the comments being added.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

  reply	other threads:[~2016-03-21 15:05 UTC|newest]

Thread overview: 124+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-15 17:56 [PATCH v4] xSplice v1 design and implementation Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 01/34] compat/x86: Remove unncessary #define Konrad Rzeszutek Wilk
2016-03-15 18:57   ` Andrew Cooper
2016-03-16 11:08   ` Jan Beulich
2016-03-17  0:44     ` Konrad Rzeszutek Wilk
2016-03-17  7:45       ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 02/34] libxc: Remove dead code (XENVER_capabilities) Konrad Rzeszutek Wilk
2016-03-15 18:04   ` Andrew Cooper
2016-03-15 18:08     ` Konrad Rzeszutek Wilk
2016-03-16 18:11   ` Wei Liu
2016-03-15 17:56 ` [PATCH v4 03/34] xsm/xen_version: Add XSM for the xen_version hypercall Konrad Rzeszutek Wilk
2016-03-18 11:55   ` Jan Beulich
2016-03-18 17:26     ` Konrad Rzeszutek Wilk
2016-03-21 11:22       ` Jan Beulich
2016-03-22 16:10         ` Konrad Rzeszutek Wilk
2016-03-22 17:54           ` Daniel De Graaf
2016-03-22 17:49   ` Daniel De Graaf
2016-03-24 15:34   ` anshul makkar
2016-03-24 19:19     ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 04/34] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane Konrad Rzeszutek Wilk
2016-03-15 18:29   ` Andrew Cooper
2016-03-15 20:19     ` Konrad Rzeszutek Wilk
2016-03-17  1:38       ` Konrad Rzeszutek Wilk
2016-03-17 14:28         ` Andrew Cooper
2016-03-18 12:36         ` Jan Beulich
2016-03-18 19:22           ` Konrad Rzeszutek Wilk
2016-03-21 12:45             ` Jan Beulich
2016-03-22 15:52               ` Konrad Rzeszutek Wilk
2016-03-22 16:06                 ` Jan Beulich
2016-03-22 18:57                   ` Konrad Rzeszutek Wilk
2016-03-22 19:28                     ` Andrew Cooper
2016-03-22 20:39                       ` Konrad Rzeszutek Wilk
2016-03-23  8:56                         ` Jan Beulich
2016-03-24  2:37                           ` Konrad Rzeszutek Wilk
2016-03-24  9:15                             ` Jan Beulich
2016-03-24 11:39                               ` Konrad Rzeszutek Wilk
2016-03-22 17:51   ` Daniel De Graaf
2016-03-15 17:56 ` [PATCH v4 05/34] libxc/libxl/python/xenstat: Use new XEN_VERSION_OP hypercall Konrad Rzeszutek Wilk
2016-03-15 18:45   ` Andrew Cooper
2016-03-16 12:31   ` George Dunlap
2016-03-16 18:11   ` Wei Liu
2016-03-17  1:08     ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 06/34] x86/arm: Add BUGFRAME_NR define and BUILD checks Konrad Rzeszutek Wilk
2016-03-15 18:54   ` Andrew Cooper
2016-03-16 11:49   ` Julien Grall
2016-03-18 12:40   ` Jan Beulich
2016-03-18 19:59     ` Konrad Rzeszutek Wilk
2016-03-21 12:49       ` Jan Beulich
2016-03-22 15:39         ` Konrad Rzeszutek Wilk
2016-03-22 15:58           ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 07/34] arm/x86: Use struct virtual_region to do bug, symbol, and (x86) exception tables Konrad Rzeszutek Wilk
2016-03-15 19:24   ` Andrew Cooper
2016-03-15 19:34     ` Konrad Rzeszutek Wilk
2016-03-15 19:51       ` Andrew Cooper
2016-03-15 20:02         ` Andrew Cooper
2016-03-16 10:33           ` Jan Beulich
2016-03-18 13:07   ` Jan Beulich
2016-03-22 20:18     ` Konrad Rzeszutek Wilk
2016-03-23  8:19       ` Jan Beulich
2016-03-23 11:17         ` Julien Grall
2016-03-23 11:21           ` Jan Beulich
2016-03-24  2:49         ` Konrad Rzeszutek Wilk
2016-03-24  9:20           ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 08/34] vmap: Make the while loop less fishy Konrad Rzeszutek Wilk
2016-03-15 19:33   ` Andrew Cooper
2016-03-17 11:49     ` Jan Beulich
2016-03-17 14:37       ` Andrew Cooper
2016-03-17 15:30         ` Jan Beulich
2016-03-17 16:06           ` Ian Jackson
2016-03-17 11:48   ` Jan Beulich
2016-03-17 16:08   ` Ian Jackson
2016-03-21 12:04     ` George Dunlap
2016-03-21 13:26       ` Jan Beulich
2016-03-21 14:22         ` George Dunlap
2016-03-21 15:05           ` Jan Beulich [this message]
2016-03-15 17:56 ` [PATCH v4 09/34] vmap: ASSERT on NULL Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 10/34] vmap: Add vmalloc_cb and vfree_cb Konrad Rzeszutek Wilk
2016-03-18 13:20   ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 11/34] xsplice: Design document Konrad Rzeszutek Wilk
2016-03-23 11:18   ` Jan Beulich
2016-03-23 20:12     ` Konrad Rzeszutek Wilk
2016-03-23 20:21       ` Konrad Rzeszutek Wilk
2016-03-24  3:15     ` Konrad Rzeszutek Wilk
2016-03-24  9:32       ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 12/34] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op Konrad Rzeszutek Wilk
2016-03-16 12:12   ` Julien Grall
2016-03-16 19:58     ` Konrad Rzeszutek Wilk
2016-03-23 13:51   ` Jan Beulich
2016-03-24  3:13     ` Konrad Rzeszutek Wilk
2016-03-24  9:29       ` Jan Beulich
2016-03-15 17:56 ` [PATCH v4 13/34] libxc: Implementation of XEN_XSPLICE_op in libxc Konrad Rzeszutek Wilk
2016-03-16 18:12   ` Wei Liu
2016-03-16 20:36     ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 14/34] xen-xsplice: Tool to manipulate xsplice payloads Konrad Rzeszutek Wilk
2016-03-16 18:12   ` Wei Liu
2016-03-15 17:56 ` [PATCH v4 15/34] xsplice: Add helper elf routines Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 16/34] xsplice: Implement payload loading Konrad Rzeszutek Wilk
2016-03-22 17:25   ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 17/34] xsplice: Implement support for applying/reverting/replacing patches Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 18/34] x86/xen_hello_world.xsplice: Test payload for patching 'xen_extra_version' Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 19/34] xsplice, symbols: Implement symbol name resolution on address Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 20/34] x86, xsplice: Print payload's symbol name and payload name in backtraces Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 21/34] xsplice: Add .xsplice.hooks functions and test-case Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 22/34] xsplice: Add support for bug frames Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 23/34] xsplice: Add support for exception tables Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 24/34] xsplice: Add support for alternatives Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 25/34] build_id: Provide ld-embedded build-ids Konrad Rzeszutek Wilk
2016-03-16 18:34   ` Julien Grall
2016-03-16 21:02     ` Konrad Rzeszutek Wilk
2016-03-17  1:12       ` Konrad Rzeszutek Wilk
2016-03-17 11:08         ` Julien Grall
2016-03-17 13:39           ` Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 26/34] HYPERCALL_version_op: Add VERSION_OP_build_id to retrieve build-id Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 27/34] libxl: info: Display build_id of the hypervisor using XEN_VERSION_OP_build_id Konrad Rzeszutek Wilk
2016-03-16 18:12   ` Wei Liu
2016-03-15 17:56 ` [PATCH v4 28/34] xsplice: Print build_id in keyhandler and on bootup Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 29/34] xsplice: Stacking build-id dependency checking Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 30/34] xsplice/xen_replace_world: Test-case for XSPLICE_ACTION_REPLACE Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 31/34] xsplice: Print dependency and payloads build_id in the keyhandler Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 32/34] xsplice: Prevent duplicate payloads from being loaded Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 33/34] xsplice: Add support for shadow variables Konrad Rzeszutek Wilk
2016-03-15 17:56 ` [PATCH v4 34/34] MAINTAINERS/xsplice: Add myself and Ross as the maintainers Konrad Rzeszutek Wilk
2016-03-16 11:10   ` Jan Beulich
2016-03-17  0:44     ` Konrad Rzeszutek Wilk

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=56F01BC302000078000DEE58@prv-mh.provo.novell.com \
    --to=jbeulich@suse.com \
    --cc=George.Dunlap@eu.citrix.com \
    --cc=Ian.Jackson@eu.citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=keir@xen.org \
    --cc=mpohlack@amazon.de \
    --cc=ross.lagerwall@citrix.com \
    --cc=sasha.levin@oracle.com \
    --cc=tim@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).