On Fri, Oct 04, 2019 at 10:53:41AM -0700, Linus Torvalds wrote: > On Fri, Oct 4, 2019 at 3:42 AM Christian Brauner > <[email protected]> wrote: > > > > The only separate fix we we had to apply > > was for a warning by clang when building the tests for using the result of > > an assignment as a condition without parantheses. > > Hmm. That code is ugly, both before and after the fix. > > This just doesn't make sense for so many reasons: > > if ((ret |= test(umem_src == NULL, "kmalloc failed"))) > > where the insanity comes from > > - why "|=" when you know that "ret" was zero before (and it had to > be, for the test to make sense) > > - why do this as a single line anyway? > > - don't do the stupid "double parenthesis" to hide a warning. Make it > use an actual comparison if you add a layer of parentheses. > > So > > if ((x = y)) > > is *wrong*. I know the compiler suggests that, but the compiler is > just being stupid, and the suggestion comes from people who don't have > any taste. > > If you want to test an assignment, you should just use > > if ((x = y) != 0) > > instead, at which point it's not syntactic noise mind-games any more, > but the parenthesis actually make sense. > > However, you had no reason to use an assignment in the conditional in > the first place. > > IOW, the code should have just been > > ret = test(umem_src == NULL, "kmalloc failed"); > if (ret) ...
Yes, I had this as the original fix but I tried to keep the same intention as the original author. I should have gone with my gut. Sorry for the ugliness, I'll try to be better in the future. Cheers, Nathan
