| Message ID | 20241224191529.138119-3-Ariel.Otilibili-Anieli@eurecom.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/libs,xen/tools: remove dead code | expand |
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@cardoe.com> > Suggested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@eurecom.fr> ... 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
diff --git a/xen/tools/kconfig/expr.c b/xen/tools/kconfig/expr.c index 77ffff3a05..84af124626 100644 --- 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 */ default: - return -1; + break; } - printf("[%dgt%d?]", t1, t2); return 0; }
The original file was imported from Linux; patched the entire expr_compare_type() with the diff from Linux. Commits wherein it might have been fixed in Linux: - dfe8e56fc604 ("kconfig: add fallthrough comments to expr_compare_type()") - 9ad86d747c46 ("kconfig: remove unreachable printf()"). ``` $ 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@cardoe.com> Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@eurecom.fr> --- xen/tools/kconfig/expr.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)