Re: [Qemu-devel] [PATCH v2] target-i386: Fix segment cache dump

[Tobias Markus]

On 08/23/2013 10:01 PM, Richard Henderson wrote:
> On 08/23/2013 12:09 PM, Tobias Markus wrote:
>> When in Long Mode, cpu_x86_seg_cache() logs "DS16" because the Default 
>> operation size bit (D/B bit) is not set for Long Mode Data Segments since 
>> there are only Data Segments in Long Mode and no explicit 16/32/64-bit 
>> Descriptors.
>> This patch fixes this by checking the Long Mode Active bit of the hidden 
>> flags variable and logging "DS" if it is set. (I.e. in Long Mode all Data 
>> Segments are logged as "DS")
>>
>> Signed-off-by: Tobias Markus <address@hidden>
> 
> Reviewed-by: Richard Henderson <address@hidden>
> 
>> +            cpu_fprintf(f, (sc->flags & DESC_B_MASK ||
>> +                        env->hflags & HF_LMA_MASK)
>> +                        ? "DS  " : "DS16");
> 
> Though we don't have anything in CODING_STYLE that mandates this,
> IMO expressions shouldn't "unindent" in the middle like this.
Other similar cpu_fprintf()s also "unindent"ed, so I aimed for consistency.
I could submit a seperate patch to properly indent the relevant lines, but that 
seems like a seperate discussion. (Would be better to add it to CODING_STYLE, 
but that is obviously non-trivial and there are possibly many more files 
affected.)
> 
> Better as
> 
>            cpu_fprintf(f, (sc->flags & DESC_B_MASK
>                            || env->hflags & HF_LMA_MASK)
>                            ? "DS  " : "DS16");
> 
> but that's just me and emacs...
> 
> 
> r~
> 
Alternatively, for readability:
+            cpu_fprintf(f, (sc->flags & DESC_B_MASK || env->hflags & 
HF_LMA_MASK)
+                            ? "DS  " : "DS16");
The upper line would be 82 characters long. I'm not sure how strictly line 
width is enforced.
(Other lines in that file (target-i386/helper.c) also exceed the width limit.)