| Message ID | 20181109143004.24963-1-jtulak@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | xfsdump: code style change | expand |
On 11/9/18 8:29 AM, Jan Tulak wrote: > This set is dealing with whitespaces only, no functional change, code > shuffling, etc. should be present (with one small exception in patch no. 2). > From the previous set I split the changes into multiple patches, one type of > change at a time, and tried to make the code fully compliant with xfs style, > so the number of changes is way bigger than in the first set. > > The only patch with code shuffling (but no behavioral change) is: "xfsdump: do > not split function call with ifdef." See the patch for details. > > For patch 21 - xfsdump: (style) format intercharacter spaces, I think whether > it is still to much agregated and I should separate the changes. Some other > patches I had to split to multiple files as they are too big to pass through > to the mailing list - it is in their name. The largest one is 660K now, so I > will check if that passed through, or it is still too big. > > The whole set is created mechanically from scratch. Scripts used for the > specific change are part of the commit message and I can easily put it all > together. So there is no need to worry about rebase conflicts, etc. when > pointing out issues. > > The tools used to make these changes are sed and uncrustify. > http://uncrustify.sourceforge.net/ > > A git tree with these patches is also on Github: > https://github.com/jtulak/xfsdump/tree/style-nov-9th > or > git clone --single-branch -b style-nov-9th https://github.com/jtulak/xfsdump.git Thanks for doing this. I do think it's useful in the long run. NB: not all patches came through, apparently they were too big for the list. A few issues I notice: 1) we now get warnings like: In file included from content.c:44:0: types.h:30:0: warning: "sizeofmember" redefined [enabled by default] #define sizeofmember(t, m) sizeof(((t *)0)->m) ^ In file included from content.c:40:0: /usr/include/xfs/jdm.h:65:0: note: this is the location of the previous definition #define sizeofmember( t, m ) sizeof( ( ( t * )0 )->m ) ^ because xfsprogs headers have the definitions w/ the spaces in them. So I'd just leave sizeofmember and offsetofmember defines alone for now to avoid this. Or, wrap them in #ifndef's. Or if we require xfsprogs headers to build anyway, perhaps just drop the defines altogether. 2) I think we probably do not want to change any user-facing strings, or at least they should be carefully examined. This will break some translations (fixable) but it's also more of a functional change, and should be done with care if at all, and maybe left for a separate patch if it's warranted. 3) This creates /many/ more > 80 char lines. Per: # for F in */*.[ch]; do expand $F | grep '.\{81\}'; done the codebase goes from 794 such lines to 1806. IOWs, some of the "missing" whitespace may have been intentional, to stay under 80 cols. New lines vs. compressed lines is a judgement call, I guess. Bleah. So... sorry about that, but if the goal is to format better, let's be sure to keep observing other basic rules like "< 80 cols" 4) related, there are places where we intentionally dropped a space to keep things well aligned, for example: ULO(_("<file> (restore only if newer than)"), GETOPT_NEWER); ULO(_("(restore owner/group even if not root)"),GETOPT_OWNER); ULO(_("<seconds between progress reports>"), GETOPT_PROGRESS); this "breaks the rule" but it's fairly common to line up columns that way if it's just off by 1 char. Other things like this: struct quota_info { - char * desc; /* Quotas type (user, project, etc) */ - bool_t savequotas; /* Quotas saved OK */ - char * quotafile; /* Filename quota info is stored in */ - char quotapath[MAXPATHLEN]; /* Full path to quotafile */ - char * repquotaargs; /* Args to repquota to create this quotafile */ - int statflag; /* quota stats flag for this type */ - ino_t quotaino; /* ino of the quota file */ + char * desc; /* Quotas type (user, project, etc) */ + bool_t savequotas; /* Quotas saved OK */ + char * quotafile; /* Filename quota info is stored in */ + char quotapath[MAXPATHLEN]; /* Full path to quotafile */ + char * repquotaargs; /* Args to repquota to create this quotafile */ + int statflag; /* quota stats flag for this type */ + ino_t quotaino; /* ino of the quota file */ }; probably shouldn't happen either, it's better to keep stuff < 80cols and have 1 oddball comment remaining. I was trying to compare objdump -d to ensure there were no other unintended runtime changes, but unfortunately there are changes which will affect that even if it's whitespace; for example whitespace changes within an assert() will change strings (the text of the assert) and their locations, and so will change the disassembly. So I'm not sure it's possible to validate "no functional changes" by binary inspection... I suppose we could try to limit whitespace changes to things that won't change disassembly first (for example, no string or assert changes) and then go back and do them, so that we could ge the bulk of the changes in with no modifications to the binary code, but ... I'm not sure if it's worth it. Sorry for the hassle, but even though it's "only whitespace" I think we want to be as careful as possible to not move anything backwards... Thanks, -Eric
On Wed, Nov 14, 2018 at 9:36 PM Eric Sandeen <sandeen@sandeen.net> wrote: > > On 11/9/18 8:29 AM, Jan Tulak wrote: > > This set is dealing with whitespaces only, no functional change, code > > shuffling, etc. should be present (with one small exception in patch no. 2). > > From the previous set I split the changes into multiple patches, one type of > > change at a time, and tried to make the code fully compliant with xfs style, > > so the number of changes is way bigger than in the first set. > > > > The only patch with code shuffling (but no behavioral change) is: "xfsdump: do > > not split function call with ifdef." See the patch for details. > > > > For patch 21 - xfsdump: (style) format intercharacter spaces, I think whether > > it is still to much agregated and I should separate the changes. Some other > > patches I had to split to multiple files as they are too big to pass through > > to the mailing list - it is in their name. The largest one is 660K now, so I > > will check if that passed through, or it is still too big. > > > > The whole set is created mechanically from scratch. Scripts used for the > > specific change are part of the commit message and I can easily put it all > > together. So there is no need to worry about rebase conflicts, etc. when > > pointing out issues. > > > > The tools used to make these changes are sed and uncrustify. > > http://uncrustify.sourceforge.net/ > > > > A git tree with these patches is also on Github: > > https://github.com/jtulak/xfsdump/tree/style-nov-9th > > or > > git clone --single-branch -b style-nov-9th https://github.com/jtulak/xfsdump.git > > Thanks for doing this. I do think it's useful in the long run. > > NB: not all patches came through, apparently they were too big for the list. Thanks for looking at it. And sorry for the missing mails, I checked it in my mailbox and there they arrived all. But apparently, Gmail and its deduplication made it look like it came through both mailing list and me in cc, even if I received CCed version only. > > A few issues I notice: > > 1) we now get warnings like: > > In file included from content.c:44:0: > types.h:30:0: warning: "sizeofmember" redefined [enabled by default] > #define sizeofmember(t, m) sizeof(((t *)0)->m) > ^ > In file included from content.c:40:0: > /usr/include/xfs/jdm.h:65:0: note: this is the location of the previous definition > #define sizeofmember( t, m ) sizeof( ( ( t * )0 )->m ) > ^ > > because xfsprogs headers have the definitions w/ the spaces in them. So > I'd just leave sizeofmember and offsetofmember defines alone for now to > avoid this. Or, wrap them in #ifndef's. Or if we require xfsprogs headers > to build anyway, perhaps just drop the defines altogether. Ok, I will drop it out. > > 2) I think we probably do not want to change any user-facing strings, > or at least they should be carefully examined. This will break some > translations (fixable) but it's also more of a functional change, and > should be done with care if at all, and maybe left for a separate patch > if it's warranted. > > 3) This creates /many/ more > 80 char lines. Per: > > # for F in */*.[ch]; do expand $F | grep '.\{81\}'; done > > the codebase goes from 794 such lines to 1806. IOWs, some of the "missing" > whitespace may have been intentional, to stay under 80 cols. New lines > vs. compressed lines is a judgement call, I guess. Bleah. > > So... sorry about that, but if the goal is to format better, let's be > sure to keep observing other basic rules like "< 80 cols" Yeah. I'm not sure there is an easy way to automate that... > > 4) related, there are places where we intentionally dropped a space to > keep things well aligned, for example: > > ULO(_("<file> (restore only if newer than)"), GETOPT_NEWER); > ULO(_("(restore owner/group even if not root)"),GETOPT_OWNER); > ULO(_("<seconds between progress reports>"), GETOPT_PROGRESS); > > this "breaks the rule" but it's fairly common to line up columns that way > if it's just off by 1 char. > > Other things like this: > > struct quota_info { > - char * desc; /* Quotas type (user, project, etc) */ > - bool_t savequotas; /* Quotas saved OK */ > - char * quotafile; /* Filename quota info is stored in */ > - char quotapath[MAXPATHLEN]; /* Full path to quotafile */ > - char * repquotaargs; /* Args to repquota to create this quotafile */ > - int statflag; /* quota stats flag for this type */ > - ino_t quotaino; /* ino of the quota file */ > + char * desc; /* Quotas type (user, project, etc) */ > + bool_t savequotas; /* Quotas saved OK */ > + char * quotafile; /* Filename quota info is stored in */ > + char quotapath[MAXPATHLEN]; /* Full path to quotafile */ > + char * repquotaargs; /* Args to repquota to create this quotafile */ > + int statflag; /* quota stats flag for this type */ > + ino_t quotaino; /* ino of the quota file */ > }; > > probably shouldn't happen either, it's better to keep stuff < 80cols > and have 1 oddball comment remaining. > > I was trying to compare objdump -d to ensure there were no other unintended > runtime changes, but unfortunately there are changes > which will affect that even if it's whitespace; for example whitespace > changes within an assert() will change strings (the text of the assert) > and their locations, and so will change the disassembly. So I'm not sure > it's possible to validate "no functional changes" by binary inspection... > > I suppose we could try to limit whitespace changes to things that won't > change disassembly first (for example, no string or assert changes) and > then go back and do them, so that we could ge the bulk of the changes in > with no modifications to the binary code, but ... I'm not sure if it's > worth it. I'll look at these issues and see what can be done/if that helps with disassembly. > > Sorry for the hassle, but even though it's "only whitespace" I think we > want to be as careful as possible to not move anything backwards... Yup, I agree. We are not talking about a web form that will work even if the layout breaks a bit. Thanks, Jan
On 11/15/18 6:40 AM, Jan Tulak wrote: > On Wed, Nov 14, 2018 at 9:36 PM Eric Sandeen <sandeen@sandeen.net> wrote: >> >> On 11/9/18 8:29 AM, Jan Tulak wrote: >>> This set is dealing with whitespaces only, no functional change, code >>> shuffling, etc. should be present (with one small exception in patch no. 2). >>> From the previous set I split the changes into multiple patches, one type of >>> change at a time, and tried to make the code fully compliant with xfs style, >>> so the number of changes is way bigger than in the first set. >>> >>> The only patch with code shuffling (but no behavioral change) is: "xfsdump: do >>> not split function call with ifdef." See the patch for details. >>> >>> For patch 21 - xfsdump: (style) format intercharacter spaces, I think whether >>> it is still to much agregated and I should separate the changes. Some other >>> patches I had to split to multiple files as they are too big to pass through >>> to the mailing list - it is in their name. The largest one is 660K now, so I >>> will check if that passed through, or it is still too big. >>> >>> The whole set is created mechanically from scratch. Scripts used for the >>> specific change are part of the commit message and I can easily put it all >>> together. So there is no need to worry about rebase conflicts, etc. when >>> pointing out issues. >>> >>> The tools used to make these changes are sed and uncrustify. >>> http://uncrustify.sourceforge.net/ >>> >>> A git tree with these patches is also on Github: >>> https://github.com/jtulak/xfsdump/tree/style-nov-9th >>> or >>> git clone --single-branch -b style-nov-9th https://github.com/jtulak/xfsdump.git >> >> Thanks for doing this. I do think it's useful in the long run. >> >> NB: not all patches came through, apparently they were too big for the list. > > Thanks for looking at it. And sorry for the missing mails, I checked > it in my mailbox and there they arrived all. But apparently, Gmail and > its deduplication made it look like it came through both mailing list > and me in cc, even if I received CCed version only. > >> >> A few issues I notice: >> >> 1) we now get warnings like: >> >> In file included from content.c:44:0: >> types.h:30:0: warning: "sizeofmember" redefined [enabled by default] >> #define sizeofmember(t, m) sizeof(((t *)0)->m) >> ^ >> In file included from content.c:40:0: >> /usr/include/xfs/jdm.h:65:0: note: this is the location of the previous definition >> #define sizeofmember( t, m ) sizeof( ( ( t * )0 )->m ) >> ^ >> >> because xfsprogs headers have the definitions w/ the spaces in them. So >> I'd just leave sizeofmember and offsetofmember defines alone for now to >> avoid this. Or, wrap them in #ifndef's. Or if we require xfsprogs headers >> to build anyway, perhaps just drop the defines altogether. > > Ok, I will drop it out. > >> >> 2) I think we probably do not want to change any user-facing strings, >> or at least they should be carefully examined. This will break some >> translations (fixable) but it's also more of a functional change, and >> should be done with care if at all, and maybe left for a separate patch >> if it's warranted. >> >> 3) This creates /many/ more > 80 char lines. Per: >> >> # for F in */*.[ch]; do expand $F | grep '.\{81\}'; done >> >> the codebase goes from 794 such lines to 1806. IOWs, some of the "missing" >> whitespace may have been intentional, to stay under 80 cols. New lines >> vs. compressed lines is a judgement call, I guess. Bleah. >> >> So... sorry about that, but if the goal is to format better, let's be >> sure to keep observing other basic rules like "< 80 cols" > > Yeah. I'm not sure there is an easy way to automate that... Well, I think that's kind of the point here. Do your automated replacement, but then manually inspect the result - you can look for 80col problems with the example above. Then manually fix up as needed. Then proceed to the next patch. Reviewers will need to look at the changes closely nyway, so you may as well do it before submission. ;) ... >> I suppose we could try to limit whitespace changes to things that won't >> change disassembly first (for example, no string or assert changes) and >> then go back and do them, so that we could ge the bulk of the changes in >> with no modifications to the binary code, but ... I'm not sure if it's >> worth it. > > I'll look at these issues and see what can be done/if that helps with > disassembly. I'm undecided about whether it's worth it. While it would be nice to be able to compare objdumps and see that there are no changes at all, if it complicates the patchset it may not be worth it. See what you think. -Eric
On Thu, Nov 15, 2018 at 5:12 PM Eric Sandeen <sandeen@sandeen.net> wrote: > > On 11/15/18 6:40 AM, Jan Tulak wrote: > > On Wed, Nov 14, 2018 at 9:36 PM Eric Sandeen <sandeen@sandeen.net> wrote: ... > >> 3) This creates /many/ more > 80 char lines. Per: > >> > >> # for F in */*.[ch]; do expand $F | grep '.\{81\}'; done > >> > >> the codebase goes from 794 such lines to 1806. IOWs, some of the "missing" > >> whitespace may have been intentional, to stay under 80 cols. New lines > >> vs. compressed lines is a judgement call, I guess. Bleah. > >> > >> So... sorry about that, but if the goal is to format better, let's be > >> sure to keep observing other basic rules like "< 80 cols" > > > > Yeah. I'm not sure there is an easy way to automate that... > > Well, I think that's kind of the point here. Do your automated replacement, > but then manually inspect the result - you can look for 80col problems with > the example above. Then manually fix up as needed. Then proceed to the next > patch. > > Reviewers will need to look at the changes closely nyway, so you may as well > do it before submission. ;) > It's about all the rebase conflicts that arises after any change in previous patches. If it was a thing I write once, then so be it. But if I have to rewrite it again after any change in one of the previous patches, well, that doesn't scale. :-) But I hope I can solve this somehow. Thanks, Jan
This set is dealing with whitespaces only, no functional change, code shuffling, etc. should be present (with one small exception in patch no. 2). From the previous set I split the changes into multiple patches, one type of change at a time, and tried to make the code fully compliant with xfs style, so the number of changes is way bigger than in the first set. The only patch with code shuffling (but no behavioral change) is: "xfsdump: do not split function call with ifdef." See the patch for details. For patch 21 - xfsdump: (style) format intercharacter spaces, I think whether it is still to much agregated and I should separate the changes. Some other patches I had to split to multiple files as they are too big to pass through to the mailing list - it is in their name. The largest one is 660K now, so I will check if that passed through, or it is still too big. The whole set is created mechanically from scratch. Scripts used for the specific change are part of the commit message and I can easily put it all together. So there is no need to worry about rebase conflicts, etc. when pointing out issues. The tools used to make these changes are sed and uncrustify. http://uncrustify.sourceforge.net/ A git tree with these patches is also on Github: https://github.com/jtulak/xfsdump/tree/style-nov-9th or git clone --single-branch -b style-nov-9th https://github.com/jtulak/xfsdump.git Jan Tulak (24): xfsdump: (style) remove trailing whitespaces xfsdump: do not split function call with ifdef xfsdump: (1/4)(style) remove spaces from parentheses xfsdump: (2/4)(style) remove spaces from parentheses xfsdump: (3/4)(style) remove spaces from parentheses xfsdump: (4/4)(style) remove spaces from parentheses xfsdump: (style) remove a space in front of comma/semicolon xfsdump: (style) remove a space in ptr dereferences xfsdump: add a space after comma and semicolon where was none xfsdump: (style) insert a newline between type and fnt name in definitions xfsdump: (style) add a space after if, switch, for, do, while xfsdump: (1/4)(style) add first empty line for multiline comments xfsdump: (2/4)(style) add first empty line for multiline comments xfsdump: (3/4)(style) add first empty line for multiline comments xfsdump: (4/4)(style) add first empty line for multiline comments xfsdump: (style) curly brackets should wrap with one space xfsdump: (1/4)(style) indent and align the code xfsdump: (2/4)(style) indent and align the code xfsdump: (3/4)(style) indent and align the code xfsdump: (4/4)(style) indent and align the code xfsdump: (style) format intercharacter spaces xfsdump: (style) format newlines xfsdump: (style) add stars to multiline comments xfsdump: (style) remove parentheses after return common/arch_xlate.c | 45 +- common/cldmgr.c | 181 +- common/cldmgr.h | 44 +- common/cleanup.c | 95 +- common/cleanup.h | 25 +- common/content.h | 179 +- common/content_common.c | 135 +- common/content_common.h | 2 +- common/content_inode.h | 505 +- common/dlog.c | 485 +- common/dlog.h | 92 +- common/drive.c | 313 +- common/drive.h | 1015 ++-- common/drive_minrmt.c | 3952 +++++++------- common/drive_scsitape.c | 5402 ++++++++++--------- common/drive_simple.c | 1441 ++--- common/exit.h | 15 +- common/fs.c | 247 +- common/fs.h | 52 +- common/getdents.c | 51 +- common/getdents.h | 2 +- common/global.c | 353 +- common/global.h | 115 +- common/hsmapi.c | 408 +- common/hsmapi.h | 88 +- common/inventory.c | 504 +- common/inventory.h | 179 +- common/lock.c | 20 +- common/lock.h | 6 +- common/main.c | 2516 ++++----- common/media.c | 218 +- common/media.h | 82 +- common/media_rmvtape.h | 30 +- common/mlog.c | 599 +-- common/mlog.h | 148 +- common/openutil.c | 98 +- common/openutil.h | 40 +- common/path.c | 238 +- common/path.h | 11 +- common/qlock.c | 248 +- common/qlock.h | 112 +- common/rec_hdr.h | 86 +- common/ring.c | 407 +- common/ring.h | 120 +- common/stream.c | 159 +- common/stream.h | 68 +- common/timeutil.c | 14 +- common/timeutil.h | 18 +- common/ts_mtio.h | 1117 ++-- common/types.h | 173 +- common/util.c | 465 +- common/util.h | 167 +- dump/content.c | 6940 ++++++++++++------------ dump/getopt.h | 81 +- dump/inomap.c | 1486 +++--- dump/inomap.h | 125 +- dump/var.c | 129 +- dump/var.h | 4 +- include/swab.h | 127 +- include/swap.h | 42 +- inventory/getopt.h | 19 +- inventory/inv_api.c | 928 ++-- inventory/inv_core.c | 152 +- inventory/inv_files.c | 32 +- inventory/inv_fstab.c | 213 +- inventory/inv_idx.c | 500 +- inventory/inv_mgr.c | 593 ++- inventory/inv_oref.c | 253 +- inventory/inv_oref.h | 259 +- inventory/inv_priv.h | 514 +- inventory/inv_stobj.c | 1318 ++--- inventory/inventory.h | 262 +- inventory/testmain.c | 429 +- invutil/cmenu.c | 673 ++- invutil/cmenu.h | 102 +- invutil/fstab.c | 613 ++- invutil/getopt.h | 22 +- invutil/invidx.c | 1634 +++--- invutil/invidx.h | 6 +- invutil/invutil.c | 1901 ++++--- invutil/invutil.h | 66 +- invutil/list.c | 201 +- invutil/list.h | 33 +- invutil/menu.c | 463 +- invutil/screen.c | 80 +- invutil/stobj.c | 880 ++- librmt/isrmt.c | 4 +- librmt/rmtabort.c | 6 +- librmt/rmtaccess.c | 15 +- librmt/rmtclose.c | 26 +- librmt/rmtcommand.c | 15 +- librmt/rmtcreat.c | 19 +- librmt/rmtdev.c | 14 +- librmt/rmtfstat.c | 31 +- librmt/rmtioctl.c | 385 +- librmt/rmtisatty.c | 13 +- librmt/rmtlib.h | 26 +- librmt/rmtlseek.c | 23 +- librmt/rmtmsg.c | 40 +- librmt/rmtopen.c | 198 +- librmt/rmtread.c | 29 +- librmt/rmtstatus.c | 42 +- librmt/rmtwrite.c | 28 +- restore/bag.c | 97 +- restore/bag.h | 71 +- restore/content.c | 10886 ++++++++++++++++++++------------------ restore/dirattr.c | 1148 ++-- restore/dirattr.h | 82 +- restore/getopt.h | 85 +- restore/inomap.c | 538 +- restore/inomap.h | 73 +- restore/mmap.c | 24 +- restore/namreg.c | 490 +- restore/namreg.h | 37 +- restore/node.c | 664 +-- restore/node.h | 47 +- restore/tree.c | 4975 +++++++++-------- restore/tree.h | 151 +- restore/win.c | 325 +- restore/win.h | 28 +- 120 files changed, 33644 insertions(+), 31156 deletions(-) -- 2.19.1