On Wed, Feb 21, 2018 at 02:48:45PM +0900, Masahiro Yamada wrote: > 2018-02-21 5:09 GMT+09:00 Ulf Magnusson <ulfalizer@xxxxxxxxx>: > > On Tue, Feb 20, 2018 at 8:52 PM, Eugeniu Rosca <erosca@xxxxxxxxxxxxxx> wrote: > >> On Tue, Feb 20, 2018 at 04:00:18PM +0100, Ulf Magnusson wrote: > >>> On Tue, Feb 20, 2018 at 05:24:01PM +0900, Masahiro Yamada wrote: > >>> > 2018-02-20 17:13 GMT+09:00 Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>: > >>> > > > >>> > > I do not like this implementation. > >>> > > The code is super ugly, the diff-stat is too much than needed. > >>> > > > >>> > > Please rewrite the code within 20 lines. > >>> > > > >>> > > >>> > For a hint, I cleaned up the code base. > >>> > https://patchwork.kernel.org/patch/10229545/ > >>> > > >>> > which should be equivalent to yours: > >>> > https://patchwork.kernel.org/patch/10226951/ > >>> > > >>> > > >>> > No 'enum print_type', please. > >>> > >>> The reason I prefer them on separate lines consistently is to avoid stuff like the following: > >>> > >>> Selected by [y]: MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y] > >>> Selected by [n]: > >>> - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || ... > >>> - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ... > >>> - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64 > >>> > >>> That looks confusing and unbalanced to me. > >> > >> I think nobody is disputing this. Masahiro's > >> https://patchwork.kernel.org/patch/10229545/ is also showing every > >> reverse dependency top level OR expression on a new row. > > > > Ohh... sorry, didn't read closely enough. > > > > Yeah, I agree that that looks like a nice and neat solution. > > > >> > >>> > >>> There are some simple ways to trim down the size of this patchset > >>> though. > >>> > >>> Eugeniu: > >>> What do you think about the following refactoring of your 3/3 patch? > >>> Maybe there's a way to have expr_print_revdep() take just a tristate > >>> too, though it's not worth it if it just grows the code elsewhere. > >>> > >> > >> Well, Masahiro requests not using 'enum print_type' and I still see it > >> in your example. > > > > One alternative would be to keep the 'revdep' flag and add an extra > > tristate parameter for the kinds of selects to print. > > Using 'enum tristate' is what was in mind, > but I do not like to mess up __expr_print() any more. > > I re-implemented this. > https://patchwork.kernel.org/patch/10231295/ > I've tested https://patchwork.kernel.org/patch/10229545/ and https://patchwork.kernel.org/patch/10231295/ and they work great for me. Thank you for this feature. > > > > > I'm not a huge fan of having parameters that turn meaningless > > depending on the values of other parameters, but it might not be > > terrible there, if we must get rid of 'enum print_type'. I'm always > > open to other suggestions too. > > > > The whole expression printing code feels a bit twisty to me with all > > the mutual recursion and stuff going on, but that's outside the scope > > of this patchset. :) > > > >> > >>> (By the way, I noticed that expr_print_revdep() previously generated a > >>> warning suggesting parentheses, which was my fault. If you saw that, > >>> don't copy my mistakes. The build should be warning-free. :) > >> > >> I didn't notice any warnings using gcc 5.4.0. > > > > I'm on 7.2.0, so that's probably why. It was "just" a style warning anyway. :) > > > > Cheers, > > Ulf > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Best Regards > Masahiro Yamada Best regards, Eugeniu. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
