[squid-dev] [PATCH] splitting typedefs.h and enums.h

[Kinkie]

On Sun, Aug 30, 2015 at 1:01 PM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> On 30/08/2015 2:20 p.m., Kinkie wrote:
> > Hi,
> >   this is a snapshot of my current effort to split typedefs.h and
> enums.h,
> > removing unneeded definitions and shuffling the remaining ones to
> locations
> > where they are actually used.
> >
> > If there are no objections, I would consider this to be a merge
> candidate,
> > although the effort is not yet completed. typedefs.h is almost complete,
> > and enums.h just started. I'd like to carry on with the latter, but it's
> > already a big patch as it is.
> >
> > Thanks
> >
>
> grumblie: I asked you to avoid touching enums during the typedefs.h scope.
>

I must do something about my English. I had understood you discouraged it,
but not this strongly.
Reverted enums.h changes


>
> What I would like to see in this patch scope is only removing the
> #include enums.h from typedefs.h and adding *only* equivalent #include
> enums.h directly into other files which break as a result. No shuffling
> of enums content themselves.
>

Can't do. IRCB takes an enum as argument; we need to get IRCB out of
typedefs to do that. Without going into the AsyncJob rework you were
referring to, where could it be shuffled to?


> All of this shuffling risks build failures and bitrot in components we
> might overlook. Which means backportage when its discovered.
>
> The bigger the patch is the more difficulty to backporting efforts when
> only a small piece of it needs to accompany a portage patch. The
> typedefs change by the nature of typedef affects far fewer portages than
> enums will. Theoretically each of these symbol changes should be in a
> different commit, but that is taking it a little far in the opposite
> direction for typedefs. (I will be requesting much lower granularity for
> enums however.)
>
> NP: yes these are mostly the same arguments we hear from Alex about HERE
> removal. And for good reason. They are valid in both discussions.
>

No problems with that. Only downside to more granularity is that it takes
more time due to wait for audits.


>
> In src/CacheDigest.h
>
> * you can remove the "for cache_key" comments now. Its obvious enough
> from the filename.
>  - the comment was only there to see why typedefs.h was still needed.
>  - same in src/tests/stub_CacheDigest.cc
>
> * kb_t comment is outdated now
>

Can't find it, where?


> In src/DiskIO/DiskThreads/DiskThreadsDiskFile.h:
>
> * please take the opportunity to add the empty line above the #include
> list.
>

Ok


> In src/SquidTime.h
>
> * s/miliseconds/milliseconds/
>
> * s/*Use/* Use/
>

Ok

>
>
> In src/comm/Connection.h
>
> * please try to keep the #include sort order alphabetical, ignore the
> #if/#endif protection when considering that manually.
>  - ip/* goes after eui/*
>  - similar in src/ipc/MemMap.h, src/ipc/StoreMap.h,
>

Ok


>
>
> In src/comm/forward.h
>
> * please take the opportunity to finally document what "PF" is:
>   /// legacy CBDATA callback functions ABI definition for read or write
> I/O events
>   /// \deprecated use CommCalls API instead where possible
>
> Ok


>
> In src/fs/forward.h
>
> * missing empty line after the copyright
> * double-empty line needs removing
> * missing empty line at end of file
>

Ok for the first two, empty line at end of file seems present to me..


> * please take the opportunity to add documentation of what the types
> shuffled into there are.
>  - probably a good idea to dig into the old wiki ProgrammingGuide or
> store code to find out if anyone has already written a good description
> documenting them somewhere else.
>

There's only PF, and you already documented it.


> In src/fs/rock/RockRebuild.cc
>
> * check if fs/rock/RockForward.h is already being included by the other
> rock/*.h files indirectly.
>  - If so fs/foward.h is unnecessary.
>  - if not then it should be #include'd instead of fs/forward.h
>

It is. Removing.


> NP: add to our TODO list that fs/rock/RockForward.h should be
> fs/rock/forward.h and lacks documentation. Sigh.
>

Either I do that now, or I suspect it won't be done. Should I?


> In src/mem/forward.h
>
> * please mark FREE with a "/// \deprecated use MEMRPROXY_CLASS instead"
>

Ok.

Updated patch attached.

  Kinkie
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cleanups-v2.patch
Type: application/octet-stream
Size: 27763 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150831/423f5558/attachment-0001.obj>