On Thu, 2023-12-07 at 14:18 +0800, Yang Yujie wrote:
> On Thu, Dec 07, 2023 at 11:02:58AM +0800, Xi Ruoyao wrote:
> >
> > I don't like this pair of {} for the for statement. It's not necessary
> > and it changes the indent level, causing the diff hard to review.
> >
> > Otherwise LGTM. I'm not sure why I didn't notice the eh_return issue
> > when I learnt shrink wrapping from RISC-V...
> >
>
> Thanks for the review! This problem on LoongArch was first noticed in a
> failed libphobos test case, and the fix is partially borrowed from i386,
> which seemed to be the only architecture without this issue.
>
> So despite the extra braces (which I'd say I prefer to have because of the
> new block of comment inserted on top of the if statement :P), I am going to
> ask Lulu for pushing this.
I understand and I don't think adding {} is wrong. The problem is the
indent change causes a large chunk of diff and it makes reviewing more
difficult. Thus generally we should not mix real code change and format
change in a commit.
i. e. it would be better to separate it into two patches, the first adds
{} and changes the indent, and the second changes the logic. But now I
don't think it's needed to make a V4, just pushing this should be fine.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
