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

[Amos Jeffries]

On 31/08/2015 8:13 p.m., Kinkie wrote:
> On Sun, Aug 30, 2015 at 1:01 PM, Amos Jeffries 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?
> 

Sorry I was not being clear still. What I meant by "only" at the
beginning was that enums.h move/remove/add as an example of the limit of
changes related to enums which might be in scope. If part (ie. removing
enume.h from typedefs.h) is not possible, then by all means avoid that too.


> 
>> 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?
> 

Sorry that was in src/ClientInfo.h on the new #include

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

lol. Now the space at the end is missing. oops.



>>
>> 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..
> 

Oh well, the maintenance tools will catch it.


>> 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?
> 

Next once this is in maybe. Or I could.

Looks okay to me. +1 if it still builds after the audit tweaks.

Amos