On 5/08/2012 12:22 a.m., Kinkie wrote:
Hi,
v2 addresses (or at least triest to) address all your suggestions,
plus it changes DBG_CRITICAL and DBG_IMPORTANT across the whole
source,
Hmm. okay. I was only requesting the new added/modified lines for now.
The source-wide should probably be done as a separate update which can
be ported to 3.2 later.
and changes CBDATA_CLASS* positions everywhere.
Does Squid still build fine? Last time I tried to move one it failed to
build cleanly afterwards. I'm not entirely confident we have unit-test
coverage to test them all either.
Was only asking to mark them for later attention so they are easy to
find again.
Due to feeping creaturism the patch name is not really correct
anymore, but I'll stick to it for now. The relative feature-branch is
lp:~kinkie/squid/fixme-fixes .
On Wed, Aug 1, 2012 at 3:01 AM, Amos Jeffries <[email protected]> wrote:
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
