From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corneliu ZUZU Subject: Re: [PATCH v2] arm/monitor vm-events: Implement guest-request support Date: Fri, 26 Feb 2016 14:53:37 +0200 Message-ID: <56D04AD1.10704@bitdefender.com> References: <1456484820-21160-1-git-send-email-czuzu@bitdefender.com> <56D04B6D02000078000D6A5A@prv-mh.provo.novell.com> <56D03F6D.9080206@bitdefender.com> <56D041C3.8000907@bitdefender.com> <56D04302.3040001@bitdefender.com> <56D053B902000078000D6AB4@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3482599101976923482==" Return-path: In-Reply-To: <56D053B902000078000D6AB4@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: Tamas K Lengyel , Keir Fraser , Ian Campbell , Razvan Cojocaru , Andrew Cooper , xen-devel@lists.xen.org, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============3482599101976923482== Content-Type: multipart/alternative; boundary="------------030708000007000502030402" This is a multi-part message in MIME format. --------------030708000007000502030402 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 2/26/2016 2:31 PM, Jan Beulich wrote: >>>> On 26.02.16 at 13:20, wrote: >> On 2/26/2016 2:14 PM, Razvan Cojocaru wrote: >>> On 02/26/2016 02:05 PM, Corneliu ZUZU wrote: >>>> On 2/26/2016 1:56 PM, Jan Beulich wrote: >>>>>>>> On 26.02.16 at 12:07, wrote: >>>>>> --- a/xen/include/asm-x86/altp2m.h >>>>>> +++ b/xen/include/asm-x86/altp2m.h >>>>>> @@ -15,8 +15,8 @@ >>>>>> * this program; If not, see . >>>>>> */ >>>>>> -#ifndef _X86_ALTP2M_H >>>>>> -#define _X86_ALTP2M_H >>>>>> +#ifndef __ASM_X86_ALTP2M_H >>>>>> +#define __ASM_X86_ALTP2M_H >>>>> Unrelated change? (No need to undo, but please don't mix such >>>>> into patches especially when they are quite large already anyway.) >>>> Noted. >>>> >>>>>> @@ -33,5 +33,9 @@ void altp2m_vcpu_initialise(struct vcpu *v); >>>>>> void altp2m_vcpu_destroy(struct vcpu *v); >>>>>> void altp2m_vcpu_reset(struct vcpu *v); >>>>>> -#endif /* _X86_ALTP2M_H */ >>>>>> +static inline uint16_t altp2m_vcpu_idx(struct vcpu *v) >>>>> const >>>> 'const', as in: >>>> >>>> +static inline const uint16_t altp2m_vcpu_idx(struct vcpu *v) >>> Since there's no functional difference between returning const uint6_t >>> and plain uint16_t, I assume that Jan meant "const struct vcpu *v". >> I thought the functional difference would be when calling: >> >> uint16_t idx = altp2m_vcpu_idx(v); // => can subsequently modify idx >> const uint16_t idx = altp2m_vcpu_idx(v); // => cannot subsequently >> modify idx (unless const is casted to non-const) > That's correct, but for this the return type of the function doesn't > matter. In fact I'd expect the compiler to warn about a meaningless > modifier placed on a function return type. > > Jan > > I find having static inline const uint16_t altp2m_vcpu_idx(struct vcpu *v) and subsequently writing uint16_t idx = altp2m_vcpu_idx(v); instead of const uint16_t idx = altp2m_vcpu_idx(v); without the compiler throwing at least a warning counter-intuitive. Reminds me of something Linus said in an email: "it would be *stupid* for a C compiler to do anything but what we assume it does". Noted, will change to 'const struct vcpu *v'. Corneliu. --------------030708000007000502030402 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 7bit
On 2/26/2016 2:31 PM, Jan Beulich wrote:
On 26.02.16 at 13:20, <czuzu@bitdefender.com> wrote:

On 2/26/2016 2:14 PM, Razvan Cojocaru wrote:

On 02/26/2016 02:05 PM, Corneliu ZUZU wrote:

On 2/26/2016 1:56 PM, Jan Beulich wrote:

On 26.02.16 at 12:07, <czuzu@bitdefender.com> wrote:

--- a/xen/include/asm-x86/altp2m.h
+++ b/xen/include/asm-x86/altp2m.h
@@ -15,8 +15,8 @@
    * this program; If not, see <http://www.gnu.org/licenses/>.
    */
   -#ifndef _X86_ALTP2M_H
-#define _X86_ALTP2M_H
+#ifndef __ASM_X86_ALTP2M_H
+#define __ASM_X86_ALTP2M_H

Unrelated change? (No need to undo, but please don't mix such
into patches especially when they are quite large already anyway.)

Noted.


@@ -33,5 +33,9 @@ void altp2m_vcpu_initialise(struct vcpu *v);
   void altp2m_vcpu_destroy(struct vcpu *v);
   void altp2m_vcpu_reset(struct vcpu *v);
   -#endif /* _X86_ALTP2M_H */
+static inline uint16_t altp2m_vcpu_idx(struct vcpu *v)

const

'const', as in:

+static inline const uint16_t altp2m_vcpu_idx(struct vcpu *v)

Since there's no functional difference between returning const uint6_t
and plain uint16_t, I assume that Jan meant "const struct vcpu *v".

I thought the functional difference would be when calling:

uint16_t idx = altp2m_vcpu_idx(v); // => can subsequently modify idx
const uint16_t idx = altp2m_vcpu_idx(v); // => cannot subsequently 
modify idx (unless const is casted to non-const)

That's correct, but for this the return type of the function doesn't
matter. In fact I'd expect the compiler to warn about a meaningless
modifier placed on a function return type.

Jan

I find having
static inline const uint16_t altp2m_vcpu_idx(struct vcpu *v)
and subsequently writing
uint16_t idx = altp2m_vcpu_idx(v);
instead of
const uint16_t idx = altp2m_vcpu_idx(v);
without the compiler throwing at least a warning counter-intuitive.

Reminds me of something Linus said in an email: "it would be *stupid* for a C compiler to do anything but what we assume it does".

Noted, will change to 'const struct vcpu *v'.

Corneliu.
--------------030708000007000502030402-- --===============3482599101976923482== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============3482599101976923482==--