| Message ID | 20240908-mips-chore-v1-2-9239c783f233@flygoat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | MIPS: Chore clean ups | expand |
On Sun, 8 Sep 2024, Jiaxun Yang wrote: > Expand the if condition into cascaded ifs to make code > readable. Apart from broken formatting what's making original code unreadable? > Also use sizeof(union mips_instruction) instead of > sizeof(mips_instruction) to match the code context. That has to be a separate change. > diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c > index dc39f5b3fb83..96139adefad2 100644 > --- a/arch/mips/kernel/kprobes.c > +++ b/arch/mips/kernel/kprobes.c > @@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p) > goto out; > } > > - if (copy_from_kernel_nofault(&prev_insn, p->addr - 1, > - sizeof(mips_instruction)) == 0 && > - insn_has_delayslot(prev_insn)) { > - pr_notice("Kprobes for branch delayslot are not supported\n"); > - ret = -EINVAL; > - goto out; > + if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) { Overlong line. > + if (insn_has_delayslot(prev_insn)) { > + pr_notice("Kprobes for branch delayslot are not supported\n"); This now overruns 80 columns making code *less* readable. Maciej
在2024年9月9日九月 下午11:02，Maciej W. Rozycki写道： > On Sun, 8 Sep 2024, Jiaxun Yang wrote: > >> Expand the if condition into cascaded ifs to make code >> readable. > > Apart from broken formatting what's making original code unreadable? For me it's confusing because wired layout, cascaded ifs are clearly easier to format and has clear intention. > >> Also use sizeof(union mips_instruction) instead of >> sizeof(mips_instruction) to match the code context. > > That has to be a separate change. Given that it's a tiny style change as well, it makes sense to combine into same patch. > >> diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c >> index dc39f5b3fb83..96139adefad2 100644 >> --- a/arch/mips/kernel/kprobes.c >> +++ b/arch/mips/kernel/kprobes.c >> @@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p) >> goto out; >> } >> >> - if (copy_from_kernel_nofault(&prev_insn, p->addr - 1, >> - sizeof(mips_instruction)) == 0 && >> - insn_has_delayslot(prev_insn)) { >> - pr_notice("Kprobes for branch delayslot are not supported\n"); >> - ret = -EINVAL; >> - goto out; >> + if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) { > > Overlong line. Nowadays, check-patch.pl is happy with 100 column line. I used 100 column line in many subsystems and never receive any complaint. > >> + if (insn_has_delayslot(prev_insn)) { >> + pr_notice("Kprobes for branch delayslot are not supported\n"); > > This now overruns 80 columns making code *less* readable. I don't really agree, we are not in VGA display era any more, see Linus's arguments on removal of 80 columns [1] and why long line are more readable [2]. [1]: https://lore.kernel.org/lkml/CAHk-=wj3iGQqjpvc+gf6+C29Jo4COj6OQQFzdY0h5qvYKTdCow@mail.gmail.com/ [2]: https://lore.kernel.org/lkml/CAHk-=wjR0H3+2ba0UUWwoYzYBH0GX9yTf5dj2MZyo0xvyzvJnA@mail.gmail.com/ Thanks > > Maciej
On Tue, Sep 10, 2024 at 10:43:23AM +0100, Jiaxun Yang wrote: > > > 在2024年9月9日九月 下午11:02，Maciej W. Rozycki写道： > > On Sun, 8 Sep 2024, Jiaxun Yang wrote: > > > >> Expand the if condition into cascaded ifs to make code > >> readable. > > > > Apart from broken formatting what's making original code unreadable? > > For me it's confusing because wired layout, cascaded ifs are clearly > easier to format and has clear intention. I prefer the original version, it's just to statements combined with &&, which isn't scary at all. Thomas.
On Tue, 10 Sep 2024, Jiaxun Yang wrote: > >> + if (insn_has_delayslot(prev_insn)) { > >> + pr_notice("Kprobes for branch delayslot are not supported\n"); > > > > This now overruns 80 columns making code *less* readable. > > I don't really agree, we are not in VGA display era any more, see Linus's > arguments on removal of 80 columns [1] and why long line are more readable [2]. Human perception hasn't changed though and just that you can squeeze a vast number of characters in a line it doesn't mean you can parse them comfortably with eyes. Printed books have a common line length limit too, established with centuries of experience. Some projects such as GDB prefer a lower soft limit of 74 characters even (with 80 being the hard one), exactly for this reason[1]. NB even back in 1990s there was an option to use 132-character lines with SVGA hardware in text mode, but hardly anybody used that because, well, it didn't make text any more readable. Rather one became lost right away. Last but not least Documentation/process/coding-style.rst still mandates 80-column formatting. References: [1] <https://sourceware.org/ml/gdb-patches/2014-01/msg00216.html> Maciej
diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c index dc39f5b3fb83..96139adefad2 100644 --- a/arch/mips/kernel/kprobes.c +++ b/arch/mips/kernel/kprobes.c @@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p) goto out; } - if (copy_from_kernel_nofault(&prev_insn, p->addr - 1, - sizeof(mips_instruction)) == 0 && - insn_has_delayslot(prev_insn)) { - pr_notice("Kprobes for branch delayslot are not supported\n"); - ret = -EINVAL; - goto out; + if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) { + if (insn_has_delayslot(prev_insn)) { + pr_notice("Kprobes for branch delayslot are not supported\n"); + ret = -EINVAL; + goto out; + } } if (__insn_is_compact_branch(insn)) {
Expand the if condition into cascaded ifs to make code readable. Also use sizeof(union mips_instruction) instead of sizeof(mips_instruction) to match the code context. Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> --- arch/mips/kernel/kprobes.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)