Re: [PATCH v2 2/2] xen/tools: remove dead code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.12.2024 20:13, Ariel Otilibili wrote:
> The original file was imported from Linux; patched the entire
> expr_compare_type() with the diff from Linux.

I'm afraid that it's quite likely that taking changes to just an isolated
function will not work very well. What's worse, ...

> Commits wherein it might have been fixed in Linux:
> - dfe8e56fc604 ("kconfig: add fallthrough comments to expr_compare_type()")
> - 9ad86d747c46 ("kconfig: remove unreachable printf()").

... these references to Linux commits then don't really help, as the isolated
changes may have different effects, and hence ...

> ```
> $ diff -u xen/xen/tools/kconfig/expr.c linux/scripts/kconfig/expr.c | \
>     sed -ne '/expr_compare_type/,/return 0/{N;p}'
> 
>  static int expr_compare_type(enum expr_type t1, enum expr_type t2)
>  {
>         if (t1 == t2)
> @@ -1106,30 +999,27 @@
>         case E_GTH:
>                 if (t2 == E_EQUAL || t2 == E_UNEQUAL)
>                         return 1;
> +               /* fallthrough */
>         case E_EQUAL:
>         case E_UNEQUAL:
>                 if (t2 == E_NOT)
>                         return 1;
> +               /* fallthrough */
>         case E_NOT:
>                 if (t2 == E_AND)
>                         return 1;
> +               /* fallthrough */
>         case E_AND:
>                 if (t2 == E_OR)
>                         return 1;
> -       case E_OR:
> -               if (t2 == E_LIST)
> -                       return 1;
> -       case E_LIST:
> -               if (t2 == 0)
> -                       return 1;
> +               /* fallthrough */
>         default:
> -               return -1;
> +               break;
>         }
> -       printf("[%dgt%d?]", t1, t2);
>         return 0;
>  }
> 
> $ cd linux/;
> $ git log --oneline -1 --pretty='%h ("%s")'
> 8155b4ef3466 ("Add linux-next specific files for 20241220")
> 
> $ git remote -v
> next    git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git 
> (fetch)
> next    git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git 
> (push)
> 
> $ cd ../xen/
> $ git log --oneline -1 --pretty='%h ("%s")'
> 6419020270 ("CHANGELOG: Mention LLC coloring feature on Arm")
> 
> $ git remote -v
> up      git://xenbits.xen.org/xen.git (fetch)
> up      git://xenbits.xen.org/xen.git (push)
> ```
> 
> Coverity-ID: 1458052
> Fixes: 8c271b7584 ("build: import Kbuild/Kconfig from Linux 4.3")
> Cc: Doug Goldstein <cardoe@xxxxxxxxxx>
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@xxxxxxxxxx>

... an actual description of the (effect of the) changes done _here_ is missing.
For example, ....

> --- a/xen/tools/kconfig/expr.c
> +++ b/xen/tools/kconfig/expr.c
> @@ -1106,26 +1106,23 @@ static int expr_compare_type(enum expr_type t1, enum 
> expr_type t2)
>       case E_GTH:
>               if (t2 == E_EQUAL || t2 == E_UNEQUAL)
>                       return 1;
> +             /* fallthrough */
>       case E_EQUAL:
>       case E_UNEQUAL:
>               if (t2 == E_NOT)
>                       return 1;
> +             /* fallthrough */
>       case E_NOT:
>               if (t2 == E_AND)
>                       return 1;
> +             /* fallthrough */
>       case E_AND:
>               if (t2 == E_OR)
>                       return 1;
> -     case E_OR:
> -             if (t2 == E_LIST)
> -                     return 1;
> -     case E_LIST:
> -             if (t2 == 0)
> -                     return 1;
> +             /* fallthrough */

... it's unclear to me why removing handling of E_OR and E_LIST is actually 
correct.

All of this said - this looks like a wording issue: You did actually take two 
full
commits (adding in - see below - at least one change of your own). May I suggest
that you take those commits individually, retaining their titles and 
descriptions,
merely adding necessary further tags (Origin: and your own S-o-b)?

>       default:
> -             return -1;
> +             break;

This change isn't part of either of the mentioned commits.

>       }
> -     printf("[%dgt%d?]", t1, t2);
>       return 0;
>  }

The "Suggested-by:" also isn't quite right imo. If anything what I suggested was
to take commits from Linux. But that's whole commits, not fragments thereof, nor
multiple of them folded (unless there's a good reason to do so). And for such
straight importing I don't think "Suggested-by:" would be quite applicable.

Jan