Re: [PATCH] Refactor ufscommon into individual include/code files

On 01.08.2012 10:20, Kinkie wrote:
> Hi all,
> the attached patch splits ufscommon.h and .cc into specific-purpose
> files, as per squid standard, and reconnects the dots.
> There are no functional changes, but it removes a few FIXMEs in the
> code. Test-suite-tested.
>
> I'm not sure if this is the right time to merge, or if it's better to
> wait after 3.2 ships, but here goes.

The testRock breakage caused by r really needs to be fixed before its
worth adding anything new to trunk. We will just end up piling in more
minor undetected bugs.

NP: Nothing but bug fixes for current build failures on FreeBSD,
OpenBSD, Windows which we can test directly will get into 3.2 now until
after release. it is 2 days overdue right now.

If this passes the 3.ALPHA-matrix test scans I'm not too worried about
it being applied to trunk. But as a refactor ("SourceLayout:" project)
it is incomplete (namespace upgrade not done with appropriate symbol
changes).

  ** please update the table in wiki SourceLayout page to track this
progress and in what sub-dir the refactoring is underway in. Will need a
new row for the fs/ufs/ specific component details

Audit ...

  ... I have mentioned debugs changes, now would be a good time to do
that for this code. but if you want to skip it this patch that is okay.

src/Makefile.am:
  - irrelevant whitespace change - please revert.

src/fs/ufs/RebuildState.cc:

  - irrelevant re-naming the file in its header comment. Please remove.
  - missing DEBUG section tag in the file header
  - missing AUTHOR tag in the file header (if you can identify original
authors easily please add, otherwise this is fine)
  - extra whitespace line after CBDATA init statement. please remove.
  - debugs level-1 please update to DBG_IMPORTANT
  - debugs level-2 and lower please update to using HERE
  - debugs level-1+ please add WARNING:/ERROR:/ etc as appropriate
  - since this is a polish patch:

+ RebuildState::RebuildState
  - please elide SP between function name and parameter list initial "("
  - please ensure consistent use of whitespace around initializer list
    ie "sd(aSwapDir), LogParser(NULL), e(NULL), fromLog(true),
_done(false)"

src/fs/ufs/RebuildState.h:
  - please move class pre-defines down the #include. If the included
files need them, they much be pre-defined in there as well.
    same for src/fs/ufs/UFSStrategy.h

src/fs/ufs/StoreFSufs.cc:
  - it would seem "DiskIO/DiskIOModule.h" inside "#if 0" is useless,
please remove that #if 0 section.

src/fs/ufs/StoreSearchUFS.cc:
  - extra whitespace line after squid.h include. please remove. same for
other files.
  - missing whitespace line after CBDATA init statement. please add
  - inconsistent use of whitespace in StoreSearchUFS::StoreSearchUFS
definition, same as for RebuildState::RebuildState

src/fs/ufs/StoreSearchUFS.h and src/fs/ufs/UFSStoreState.h and
src/fs/ufs/UFSSwapDir.h and src/fs/ufs/UFSSwapLogParser.h
  - excess whitespace lines in file tail. please reduce to one empty
line. same for other .h files
  - extra line before "public:". same for a lot of the class
definitions. please remove
  - src/fs/ufs/UFSSwapLogParser.h also has class predefine before
#includes

src/fs/ufs/UFSSwapDir.cc:
  - please remove double whitespace lines between definitions.
  - please remove empty first-line inside function definitions.
  - please fix documentation comments on all functions to doxygen-style
and omit obsolete symbol names
  - please fix whitespace alignment on initializer list of
UFSSwapDir::UFSSwapDir, also please make it a vertical list 8-space
indented
  - debugs level 0 and 1, please prefix with WARNING/ERROR as
appropriate and use HERE on level-2+

src/fs/ufs/UFSSwapLogParser.cc:
  - please remove excess empty lines: one after squid.h, one after class
definitions, several at file end.

src/fs/ufs/store_dir_ufs.cc:
  - appears to have been left containing a useless #define. Please
remove that.

src/tests/testUfs.cc:
  - please remove excess whitespace line after squid.h

Across the board:
  - please add \bug comments in all classes where CBDATA_CLASS
definition is not last in the class{} definition. The macro plays tricks
with public/private which screws up what the .h file *looks* like it is
defining things as.

There is a couple of major TODO missing in those spots as well:
// TODO: move CBDATA_CLASS macro down to last as ensure the classes
still build fine (nothing was silently depending on the
functions/members defined after it being public:).

Amos
Received on Wed Aug 01 2012 - 01:01:36 MDT