On Wed, Jul 12, 2023 at 09:44:40AM +0800, yunchuan wrote: > On 2023/7/12 01:50, Simon Horman wrote: > > > - struct sock *sk = (struct sock *)chan->private; > > > + struct sock *sk = chan->private; > > > struct pppox_sock *po = pppox_sk(sk); > > > struct net_device *dev = po->pppoe_dev; > > Hi, > > > > Please don't break reverse xmas tree ordering - longest line to shortest - > > of local variable declarations in Networking code. > > Hi, > > This can't be reversed because it depends on the first declaration. > Should I change it like this? > > - struct sock *sk = (struct sock *)chan->private; > - struct pppox_sock *po = pppox_sk(sk); > + struct pppox_sock *po = pppox_sk(chan->private); > struct net_device *dev = po->pppoe_dev; > + struct sock *sk = chan->private; > > But this seems to be bad. As the advice of Andrew[1] and Dan[2]: > > " > > When dealing with existing broken reverse Christmas tree, please don't > make it worse with a change. But actually fixing it should be in a > different patch. > > We want patches to be obviously correct. By removing the cast and > moving variables around, it is less obvious it is correct, than having > two patches. > > " Thanks, I agree this is a good approach.
