[squid-dev] [PATCH] Non-zeroing mempools

[Kinkie]

>
> >
> >> * Please make the initializer lists one member per line.
> >>  - the auto-formatting wont do that for us yet.
> >>  - it greatly helps readability and manual checking for missing or
> >> out-of-order items. We can also then easily add comments to lines
> >> explain odd default values.
> >>
> >
> > Sure for .cc, not so sure for .h, where compactness helps readability and
> > presence and order should be easier to spot. Finally, many missing or
> > out-of-order should get caught either by Coverity or gcc.
> > Changed for all .cc, not done for .h (where lists are quite shorter)
> >
>
> Okay. As we kind of spoke of in IRC it is a "does it look readable" call
> for .h.
>
> The change to src/URL.h and src/acl/Acl.h are good examples of what I
> would like to avoid.
>

Moved offending ones to .cc files. You are right.


>
> - The orginal single-liner with {} at the end looks fine.
>
> - A one-per-line also looks fine.
>
> But with the slightly longer line they have now a partial wrapping in
> the set mid-way somewhere makes it hard to read.
>

Nod.


>
>
> > * in memset() please use 0 integer value not '\0' to set flags.
> >>  - it helps the compiler know it is safe to optimize up to larger than
> >> 8-bit values of 0.
> >>
> >
> > Ok.
> >
>
> I meant just for the ones you were adding. But since this is a patch
> scoped at correcting initialization issues I'll accept the ones you are
> now changing.
>

Ok


>
>
> >
> >> * src/HttpHeader.cc - revert
> >>
> >
> > Yes.
> >
>
> Now you've got src/ident/AclIdent.h with gratuitous change.
>
> I like the change itself but its well outside any relevance.
>

Dropped.


> >
> >> * when adding a new constructor ensure the big-5 operators (ctor, dtor,
> >> assign, emplace, emplace-assign) are all present. Even if they use "=
> >> default"
> >>  - looking at StoreMeta, maybe others.
> >>
> >
> > StoreMeta is pure-virtual; Also, it has no complex members so emplace
> > doesn't add value vs. copy.
> > Omitting emplace and implementing the others as protected for
> completeness'
> > sake.
> >
>
> Hmm. We are going to have to decide what to do about cases like that
> where Big-3 is more appropriate than Big-5.
>
> I'm inclined to use emplace =default/=delete anyway just for code clarity.
>
> Might be worth adding a comment to that effect just to record that
> StoreMeta has actually been considered for big-5 already.
>

I like Alex' proposal: it's as simple as it can be, consistent and it seems
to cover all cases nicely.


> >
> >> * wordlist missing 'explicit' on the raw-pointer ctor
> >>  - also other big-5 operators
> >>
> >
> > wordlist must die; it's a C relic which doesn't belong to squid anymore.
> > Kinda-implemented with a private default destructor (friended by
> > wordlistDestroy) and deleted assignment and copyconstructor. This helped
> > catch a leak in FtpGateway.cc so at least it was used for something -even
> > if it's very ugly.
> >
>
> I'm resisting the urge to request this be split to a different patch. :-P
>

Why not? It makes sense and I expect it to be self-contained.
I'll try.



> >> * please do one of these:
> >>
> >>  + return the XXX to Debugs.h, but it can move up to explain why
> >> AllocatorProxy.h is used instead of mem/forward.h
> >>
> >
> > It doesn't need to. I'm dumb and should have used mem/forward in the
> first
> > place - you had even told me not too long ago.
>
> It does. The problem was documented in the old XXX:
>
>  "pulls in typedefs.h and enums.h and globals.h"
>
> typedefs.h/enums.h/defines.h/globals.h is a nasty nest of recursive
> include worms. In particular including them from something widely used
> like Debugs.h makes it super nasty doing test builds trying to verify
> future shuffling of their contents.
>  It was weeks of effort to get defines.h out of Debugs.h in the first
> place.
>
> Thats why the original XXX existed (as it said). If you just use
> mem/forward.h there without the shuffling of FREE typedef and remove
> typedefs.h dependency, then the problem is just delayed to a subtle more
> nasty issue later.
>
> Looking at the scope creep that would may involve linkage fixes where
> things already depend on typedefs.h includes via mem/forward.h. So I
> retract mem/forward.h as an option to be selected. Please do the
> AllocatorProxy.h include with a new XXX.
>

On the plus side, typedefs.h is no longer the beast it used to be, and can
for sure be reduced further. Not doing that here now, but keeping that in
mind.



> More new bits:
>
> in src/fs/ufs/UFSStoreState.h
> * you are changing _queued_write lines but not NULL/nullptr
>

Ok


>
>
> in src/mem/AllocatorProxy.h:
> * please avoid using the term "store" in MEMPROXY_CLASS description.
>  - it kind of collides with our use of the word for caching.
>  - "memory" or "memory block" would be better here.
>

Memory block it is.


>
>
> * AllocatorProxy has tabs in its initializer list.
>

Ok.


> (thats what I'm seeing with a read-thru, haven't tried Alex's script yet).
>

Would it make sense to add that script to the tree? IMO it does if we want
to apply it consistently.


-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150824/40c41111/attachment-0001.html>