From: Sean Christopherson <seanjc@google.com>
To: David Woodhouse <dwmw2@infradead.org>
Cc: Ivan Orlov <iorlov@amazon.com>,
bp@alien8.de, dave.hansen@linux.intel.com, mingo@redhat.com,
pbonzini@redhat.com, shuah@kernel.org, tglx@linutronix.de,
hpa@zytor.com, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
x86@kernel.org, jalliste@amazon.com, nh-open-source@amazon.com,
pdurrant@amazon.co.uk
Subject: Re: [PATCH 3/3] selftests: KVM: Add test case for MMIO during event delivery
Date: Thu, 17 Oct 2024 09:58:03 -0700 [thread overview]
Message-ID: <ZxFCG7pxWXs1D0p5@google.com> (raw)
In-Reply-To: <bd116c27908111619b6cfffbe9a25e98e0e7cc20.camel@infradead.org>
On Thu, Oct 17, 2024, David Woodhouse wrote:
> On Fri, 2024-10-11 at 17:21 -0700, Sean Christopherson wrote:
> >
> > > +
> > > + /* We should never reach this point */
> >
> > No pronouns. Yes, it's nitpicky, but "we" gets _very_ ambiguous when "we" could
> > mean the admin, the user, the VMM, KVM, the guest, etc.
> >
> > > + GUEST_ASSERT(0);
>
>
> Is there really *any* way that can be interpreted as anything other
> than "the CPU executing this code will never get to this point and
> that's why there's an ASSERT(0) right after this comment"?
>
> I don't believe there's *any* way that particular pronoun can be
> ambiguous, and now we've got to the point of fetishising the bizarre
> "no pronouns" rule just for the sake of it.
No, it's not just for the sake of it. In this case, "we" isn't all that ambiguous,
(though my interpretation of it is "the test", not "the CPU"), but only because the
comment is utterly useless. The GUEST_ASSERT(0) communicates very clearly that it's
supposed to be unreachable.
And if the comment were rewritten to explain _why_ the code is unreachable, then
"we" is all bug guaranateed to become ambiguous, because explaining "why" likely
means preciesly describing the behavior the userspace side, the guest side, and/or
KVM. In other words, using "we" or "us" is often a hint that either the statement
is likely ambiguous or doesn't add value.
And irrespective of whether or not you agree with the above, having a hard rule of
"no we, no us" eliminates all subjectivity, and for me that is sufficient reason
to enforce the rule.
> I get it, especially for some individuals it *can* be difficult to take
> context into account, and the wilful use of pronouns instead of
> spelling things out explicitly *every* *single* *time* can sometimes
> help. But at a cost of conciseness and brevity.
In this particular case, I am more than willing to sacrifice brevity. I 100%
agree that there is value in having to-the-point comments and changelogs, but I
can't recall a single time where avoiding a "we" or "us" made a statement
meaningfully harder to read and understand. On ther hand, I can recall many, many
changelogs I had to re-read multiple times because I struggled to figure out how
the author _intended_ "we" or "us" to be interpreted.
prev parent reply other threads:[~2024-10-17 16:58 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-27 16:16 [PATCH 0/3] Handle MMIO during event delivery error on SVM Ivan Orlov
2024-09-27 16:16 ` [PATCH 1/3] KVM: x86, vmx: Add function for event delivery error generation Ivan Orlov
2024-10-11 23:20 ` Sean Christopherson
2024-10-12 0:06 ` Sean Christopherson
2024-10-15 19:52 ` Ivan Orlov
2024-10-16 21:05 ` Sean Christopherson
2024-10-16 22:05 ` Ivan Orlov
2024-10-16 23:12 ` Sean Christopherson
2024-09-27 16:16 ` [PATCH 2/3] KVM: vmx, svm, mmu: Process MMIO during event delivery Ivan Orlov
2024-10-12 0:05 ` Sean Christopherson
2024-10-16 22:53 ` Ivan Orlov
2024-10-17 16:20 ` Sean Christopherson
2024-09-27 16:16 ` [PATCH 3/3] selftests: KVM: Add test case for " Ivan Orlov
2024-10-12 0:21 ` Sean Christopherson
2024-10-17 16:27 ` David Woodhouse
2024-10-17 16:58 ` Sean Christopherson [this message]
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=ZxFCG7pxWXs1D0p5@google.com \
--to=seanjc@google.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=dwmw2@infradead.org \
--cc=hpa@zytor.com \
--cc=iorlov@amazon.com \
--cc=jalliste@amazon.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=nh-open-source@amazon.com \
--cc=pbonzini@redhat.com \
--cc=pdurrant@amazon.co.uk \
--cc=shuah@kernel.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.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