On 04.02.2022 23:07, George Dunlap wrote: > On Mon, Jul 5, 2021 at 5:06 PM Jan Beulich <[email protected]> wrote: > >> p2m_add_page() is simply a rename from guest_physmap_add_entry(). >> p2m_remove_page() then is its counterpart, despite rendering >> guest_physmap_remove_page().
First of all: It has been long ago that I noticed that this sentence misses words. It now ends "... a trivial wrapper." >> This way callers can use suitable pairs of >> functions (previously violated by hvm/grant_table.c). >> > > Obviously this needs some clarification. While we're here, I find this a > bit confusing; I tend to use the present tense for the way the code is > before the patch, and the imperative for what the patch does; so Id' say: > > Rename guest_physmap_add_entry() to p2m_add_page; make > guest_physmap_remove_page a wrapper with p2m_remove_page. That way callers > can use suitable pairs... Well, yes, I understand you might word it this way. I'm not convinced of the fixed scheme you mention for present vs imperative use to be a universal fit though, requiring to always be followed. When reading the description with the title in mind (and with the previously missing words added), I find the use of present tense quite reasonable here. I'm further slightly puzzled by you keeping the use of present tense in "That way callers can use ...". Jan
