Re: [Qemu-devel] [PATCH v6 04/16] tcg/ppc: Enable tcg backend vector compilation

[Aleksandar Markovic]

On Jun 30, 2019 12:48 PM, "Richard Henderson" <address@hidden>
wrote:
>
> On 6/30/19 11:46 AM, Aleksandar Markovic wrote:
> >
> >
> > On Saturday, June 29, 2019, Richard Henderson <
address@hidden
> > <mailto:address@hidden>> wrote:
> >
> >     Introduce all of the flags required to enable tcg backend vector
support,
> >     and a runtime flag to indicate the host supports Altivec
instructions.
> >
> >
> > If two flags have different purpose and usage, it is better that they
> > have different names. (perhaps one of them should have the suffix
“_runtime“)
>
> Huh?  They do have different names.  Very different names.
>

They do. If you leave the same name, you would make any search for that
name during future debugging/development more difficult.

> > Also, I am not sure if Altiveec can be reffered as isa, it is a
part/extension
> > of an isa, so “isa” seems  superfluous here.
>
> It also matches the other existing names, so I'll leave it as is.
>

If something is wrong in the old code, it does not mean one should continue
the same practice.

> > checkpatch warning should also be honored.
>
> It's bogus.

I don't think it is bogus. The comment should be converted to a regular
one-line ot perhaps multiline comment before if-statement. Althoug it may
be correct in the sense of C-syntax, noone expects coment to be inlined
into if-condition, and it makes the code feel obfuscated, rather than clear.

> > WARNING: Block comments use a leading /* on a separate line
> > #155: FILE: tcg/ppc/tcg-target.inc.c:2842:
> > +    if (hwcap & /* PPC_FEATURE_HAS_ALTIVEC -- NOT YET */ 0) {
>
> It's not a block comment; the whole thing is on one line.
> I have no idea why it doesn't notice.
>
> In any case, this goes away in patch 13.
>
>
> r~