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.
