On Jun 30, 2019 12:48 PM, "Richard Henderson" <richard.hender...@linaro.org>
wrote:
>
> On 6/30/19 11:46 AM, Aleksandar Markovic wrote:
> >
> >
> > On Saturday, June 29, 2019, Richard Henderson <
richard.hender...@linaro.org
> > <mailto:richard.hender...@linaro.org>> 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~
