<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">><br>
>> * Please make the initializer lists one member per line.<br>
>>  - the auto-formatting wont do that for us yet.<br>
>>  - it greatly helps readability and manual checking for missing or<br>
>> out-of-order items. We can also then easily add comments to lines<br>
>> explain odd default values.<br>
>><br>
><br>
> Sure for .cc, not so sure for .h, where compactness helps readability and<br>
> presence and order should be easier to spot. Finally, many missing or<br>
> out-of-order should get caught either by Coverity or gcc.<br>
> Changed for all .cc, not done for .h (where lists are quite shorter)<br>
><br>
<br>
</span>Okay. As we kind of spoke of in IRC it is a "does it look readable" call<br>
for .h.<br>
<br>
The change to src/URL.h and src/acl/Acl.h are good examples of what I<br>
would like to avoid.<br></blockquote><div><br></div><div>Moved offending ones to .cc files. You are right.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- The orginal single-liner with {} at the end looks fine.<br>
<br>
- A one-per-line also looks fine.<br>
<br>
But with the slightly longer line they have now a partial wrapping in<br>
the set mid-way somewhere makes it hard to read.<br></blockquote><div><br></div><div>Nod.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
<br>
> * in memset() please use 0 integer value not '\0' to set flags.<br>
>>  - it helps the compiler know it is safe to optimize up to larger than<br>
>> 8-bit values of 0.<br>
>><br>
><br>
> Ok.<br>
><br>
<br>
</span>I meant just for the ones you were adding. But since this is a patch<br>
scoped at correcting initialization issues I'll accept the ones you are<br>
now changing.<br></blockquote><div><br></div><div>Ok</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
<br>
><br>
>> * src/HttpHeader.cc - revert<br>
>><br>
><br>
> Yes.<br>
><br>
<br>
</span>Now you've got src/ident/AclIdent.h with gratuitous change.<br>
<br>
I like the change itself but its well outside any relevance.<br></blockquote><div><br></div><div>Dropped.</div><div>  </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
><br>
>> * when adding a new constructor ensure the big-5 operators (ctor, dtor,<br>
>> assign, emplace, emplace-assign) are all present. Even if they use "=<br>
>> default"<br>
>>  - looking at StoreMeta, maybe others.<br>
>><br>
><br>
> StoreMeta is pure-virtual; Also, it has no complex members so emplace<br>
> doesn't add value vs. copy.<br>
> Omitting emplace and implementing the others as protected for completeness'<br>
> sake.<br>
><br>
<br>
</span>Hmm. We are going to have to decide what to do about cases like that<br>
where Big-3 is more appropriate than Big-5.<br>
<br>
I'm inclined to use emplace =default/=delete anyway just for code clarity.<br>
<br>
Might be worth adding a comment to that effect just to record that<br>
StoreMeta has actually been considered for big-5 already.<br></blockquote><div><br></div><div>I like Alex' proposal: it's as simple as it can be, consistent and it seems to cover all cases nicely.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
><br>
>> * wordlist missing 'explicit' on the raw-pointer ctor<br>
>>  - also other big-5 operators<br>
>><br>
><br>
> wordlist must die; it's a C relic which doesn't belong to squid anymore.<br>
> Kinda-implemented with a private default destructor (friended by<br>
> wordlistDestroy) and deleted assignment and copyconstructor. This helped<br>
> catch a leak in FtpGateway.cc so at least it was used for something -even<br>
> if it's very ugly.<br>
><br>
<br>
</span>I'm resisting the urge to request this be split to a different patch. :-P<br></blockquote><div><br></div><div>Why not? It makes sense and I expect it to be self-contained.</div><div>I'll try.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>> * please do one of these:<br>
>><br>
>>  + return the XXX to Debugs.h, but it can move up to explain why<br>
>> AllocatorProxy.h is used instead of mem/forward.h<br>
>><br>
><br>
> It doesn't need to. I'm dumb and should have used mem/forward in the first<br>
> place - you had even told me not too long ago.<br>
<br>
</span>It does. The problem was documented in the old XXX:<br>
<br>
 "pulls in typedefs.h and enums.h and globals.h"<br>
<br>
typedefs.h/enums.h/defines.h/globals.h is a nasty nest of recursive<br>
include worms. In particular including them from something widely used<br>
like Debugs.h makes it super nasty doing test builds trying to verify<br>
future shuffling of their contents.<br>
 It was weeks of effort to get defines.h out of Debugs.h in the first place.<br>
<br>
Thats why the original XXX existed (as it said). If you just use<br>
mem/forward.h there without the shuffling of FREE typedef and remove<br>
typedefs.h dependency, then the problem is just delayed to a subtle more<br>
nasty issue later.<br>
<br>
Looking at the scope creep that would may involve linkage fixes where<br>
things already depend on typedefs.h includes via mem/forward.h. So I<br>
retract mem/forward.h as an option to be selected. Please do the<br>
AllocatorProxy.h include with a new XXX.<br></blockquote><div><br></div><div>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.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
More new bits:<br>
<br>
in src/fs/ufs/UFSStoreState.h<br>
* you are changing _queued_write lines but not NULL/nullptr<br></blockquote><div><br></div><div>Ok</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
in src/mem/AllocatorProxy.h:<br>
* please avoid using the term "store" in MEMPROXY_CLASS description.<br>
 - it kind of collides with our use of the word for caching.<br>
 - "memory" or "memory block" would be better here.<br></blockquote><div><br></div><div>Memory block it is.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
* AllocatorProxy has tabs in its initializer list.<br></blockquote><div><br></div><div>Ok.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(thats what I'm seeing with a read-thru, haven't tried Alex's script yet).<br></blockquote><div><br></div><div>Would it make sense to add that script to the tree? IMO it does if we want to apply it consistently.</div></div><br clear="all"><div><br></div>-- <br><div class="gmail_signature">    Francesco</div>
</div></div>