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
