[squid-dev] [PATCH] VIA creation code duplication

[Amos Jeffries]

On 17/03/2017 6:17 a.m., Alex Rousskov wrote:
> On 03/16/2017 05:15 AM, Amos Jeffries wrote:
> 
> 
>> Any objections to applying this with this added:
>>
>>   // XXX: putStr() still has String 64KB limits
>>   Must(strVia.length() < 64*1024);
> 
> No objections from me if you replace the magic constant with a new
> inlined String::MaxSizeXXX() method. The slightly misleading source code
> comment above becomes unnecessary after that. Why XXX()? Because no
> correct caller code should know about internal String limits (but it may
> catch exceptions, including exceptions due to those limits).

Okay.


If you dont mind spending a bit more time to chew that philosophical
point over, I am in slight disagreement that the need for XXX after
avoiding the magic number.

Your argument is good for the case where it is using magic numbers, even
when hidden behind a macro name. That is certainly a slippery slope to
bad news.

However, it seems reasonable for a caller to expect there to be _a_
limit. It is also reasonable for the caller to be aware of some API way
to find it out.

The safety in the latter comes from there being no knowledge of it being
a specific fixed value. For instance it could be changed each lookup or
depend on the currently stored String contents for all the caller knows
or cares - at the needed instant of lookup it is the value provided by
the API, that is all that matters to the caller.
 By using a member which does not require recompiling the caller if it
changes we move from the former bad situation to the latter safe(r) one.


> 
>> (I kind of hate those low-level operations with Must() and assert() like
>> the former - far too little context to be useful debugging with such big
>> impact when they are triggered.)
> 
> While it is indeed tempting to hate those Must(), we must keep in mind
> that low-level assert-replacing Must()s are not meant for debugging or
> even traditional error handling. Their purpose is to (hopefully) prevent
> Squid death.
> 
> When the crash avoidance project completes, we will have a way to dump
> stack traces for specific Must()s, which will provide the same level of
> triage info as an assert() would, but (hopefully) without killing Squid
> in environments where such functionality is supported.
> 

Any kind of estimate on that?

Amos