2016-07-02 1:08 GMT+08:00 Ralf Ramsauer <[email protected]>: > Hi Xuguo, > > thanks for the patch. > I now had a really deep look at them and still some remarks (i'm sorry). > If you write a second, third or nth revision of your patch, please keep > the original commit message as this will be the message that eventually > appears in the repository. > You can maintain a change log inside the commit message that let's us > know what happened across revisions. Here's [1] an example that gives > you a feeling on how it should be done. And as you can see, his patch > apparently took even ten rounds :-) > > Addressing all: > jailhouse-config-create contains tons of PEP8 warnings. Furthermore, > jailhouse-config-create intermixes class definitions, function > definitions and general code. There are some general code fragments in > the beginning, some at the end. We should work on that and tidy up.
Ranf, could you tell me about the "intermixes class definithins, function definitions and general codes" clearly, where? Thank you Ralf. > > But this is not urgent and can wait until Xuguo's patch is ready for > merge, otherwise he will have annoying rebase conflicts. > > Ralf > > [1] > https://github.com/torvalds/linux/commit/c5abbba932a4afd82cb35f3d6e691a2b4a0613be > > On 07/01/16 05:57, Xuguo Wang wrote: >> *** >> All remarks: >> 1 Add a whitespace after the comma ', ' >> done >> 2 # parse /proc/ioports >> omit, self-explanatory >> 3 # region is larger than two bytes >> done >> 4 overlay != overlap... >> done >> 5 Now that I see it: >> I can find these three lines of code six times in this >> patch. Isn't >> there a chance to consolidate? >> It's always the same repeating snippet: >> refactored >> 6 Amend to sth like 'pad with 0xff, if the last address is not >> 0xffff' >> done >> 7 This comment doesn't match to the next line. The next line >> does not >> parse the IOMemRegions, it defines a class... This comment >> is - in my >> opinion - misleading. >> omit, since the code self-explanatory >> 8 E.g., I'm not okay with patch 2/3, as it contains a lot of >> redundant >> code. >> refactor >> 9 Comments help, they are useful and necessary for others to >> understand your code. >> refactor >> >> valentine >> 1 Any reason for using old-style objects instead of 'class >> IORegion(object)' here? >> I think these patch just add the functional of >> pio_bitmap generator. >> 2 Having regionaclass as the first argument here strongly >> suggests you want >> @classmethod, not @staticmethod here. Is there a way to >> refactor your code? >> This could be done in the next patch. >> >> jan >> 1 Please only repost updates when you are done with them, and >> it makes >> sense for the reviewer to look at them again. Otherwise, >> they will get >> frustrated and spend their time on other things. >> done >> *** >> >> Xuguo Wang (3): >> tools: Amend the comments. >> tools: Refactor the parse_ports. >> tools: Template refactor. >> >> tools/jailhouse-config-create | 407 >> +++++++++++++++++++++++++++++++++++++----- >> tools/root-cell-config.c.tmpl | 21 +-- >> 2 files changed, 369 insertions(+), 59 deletions(-) >> > > -- > Ralf Ramsauer > PGP: 0x8F10049B 2016-07-02 10:19 GMT+08:00 charles king <[email protected]>: > OK, sorry for these messed up comments, I am learning and catching up. > Thank you for you patience Ralf and teaching. > > best regards > from Xuguo Wang > > 2016-07-02 1:08 GMT+08:00 Ralf Ramsauer <[email protected]>: >> Hi Xuguo, >> >> thanks for the patch. >> I now had a really deep look at them and still some remarks (i'm sorry). >> If you write a second, third or nth revision of your patch, please keep >> the original commit message as this will be the message that eventually >> appears in the repository. >> You can maintain a change log inside the commit message that let's us >> know what happened across revisions. Here's [1] an example that gives >> you a feeling on how it should be done. And as you can see, his patch >> apparently took even ten rounds :-) >> >> Addressing all: >> jailhouse-config-create contains tons of PEP8 warnings. Furthermore, >> jailhouse-config-create intermixes class definitions, function >> definitions and general code. There are some general code fragments in >> the beginning, some at the end. We should work on that and tidy up. >> >> But this is not urgent and can wait until Xuguo's patch is ready for >> merge, otherwise he will have annoying rebase conflicts. >> >> Ralf >> >> [1] >> https://github.com/torvalds/linux/commit/c5abbba932a4afd82cb35f3d6e691a2b4a0613be >> >> On 07/01/16 05:57, Xuguo Wang wrote: >>> *** >>> All remarks: >>> 1 Add a whitespace after the comma ', ' >>> done >>> 2 # parse /proc/ioports >>> omit, self-explanatory >>> 3 # region is larger than two bytes >>> done >>> 4 overlay != overlap... >>> done >>> 5 Now that I see it: >>> I can find these three lines of code six times in this >>> patch. Isn't >>> there a chance to consolidate? >>> It's always the same repeating snippet: >>> refactored >>> 6 Amend to sth like 'pad with 0xff, if the last address is >>> not 0xffff' >>> done >>> 7 This comment doesn't match to the next line. The next line >>> does not >>> parse the IOMemRegions, it defines a class... This comment >>> is - in my >>> opinion - misleading. >>> omit, since the code self-explanatory >>> 8 E.g., I'm not okay with patch 2/3, as it contains a lot of >>> redundant >>> code. >>> refactor >>> 9 Comments help, they are useful and necessary for others to >>> understand your code. >>> refactor >>> >>> valentine >>> 1 Any reason for using old-style objects instead of 'class >>> IORegion(object)' here? >>> I think these patch just add the functional of >>> pio_bitmap generator. >>> 2 Having regionaclass as the first argument here strongly >>> suggests you want >>> @classmethod, not @staticmethod here. Is there a way to >>> refactor your code? >>> This could be done in the next patch. >>> >>> jan >>> 1 Please only repost updates when you are done with them, and >>> it makes >>> sense for the reviewer to look at them again. Otherwise, >>> they will get >>> frustrated and spend their time on other things. >>> done >>> *** >>> >>> Xuguo Wang (3): >>> tools: Amend the comments. >>> tools: Refactor the parse_ports. >>> tools: Template refactor. >>> >>> tools/jailhouse-config-create | 407 >>> +++++++++++++++++++++++++++++++++++++----- >>> tools/root-cell-config.c.tmpl | 21 +-- >>> 2 files changed, 369 insertions(+), 59 deletions(-) >>> >> >> -- >> Ralf Ramsauer >> PGP: 0x8F10049B -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
