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

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 <squid3_at_treenet.co.nz> 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
>>
>
>
Received on Sun Aug 05 2012 - 11:21:52 MDT