[squid-dev] [PATCH] mime unfolding

[Alex Rousskov]

On 05/19/2016 07:29 AM, Amos Jeffries wrote:
> On 19/05/2016 1:59 p.m., Alex Rousskov wrote:

(A)

>>     while (!tk.atEnd()) {
>>         const SBuf all = tk.remaining();
>>         const auto crLen = tk.skipOne(CR); // may not be there
>>         const auto lfLen = tk.skipOne(LF); // may not be there
>>         if (lfLen && tk.skipAll(WSP)) // obs-fold!
>>             result += ' '; // replace one obs-fold with one SP
>>         else
>>             result += all.substr(0, crLen+lfLen + tk.skipAll(notCRLF));
>>     }

- a little too complex
+ only one expensive operation (+=) per iteration
+ elegant / symmetric / succinct


(B)

>  while (!tk.atEnd()) {
>    SBuf line;
>    if (tk.prefix(line, nonCRLF))
>         result += line;
> 
>    const SBuf all = tk.remaining();
> 
>    const auto crLen = tk.skipOne(CR); // may not be there
>    const auto lfLen = tk.skipOne(LF); // may not be there
>    if (lfLen && tk.skipAll(WSP)) // obs-fold!
>      result += ' '; // replace one obs-fold with one SP
>    else // bare-CR or CRLF
>      result += all.substr(0, crLen+lfLen);
>  }

- a little too complex
- two expensive operations (+=) per iteration
* considered clearer by some but not others

On a performance-sensitive path, (A) would have been a clear winner.
Since we are not dealing with that path, please pick (A) or (B) given
the factors above.


FYI: The "may not be there" comments became a little misleading in (B)
because either CR or LF has to be there now (in LF-terminated MIME), but
I cannot suggest a better way to phrase them without making them too
long/confusing. Not important.


> 6) 
> 
>  while (!tk.atEnd()) {
>    if (tk.prefix(line, nonCRLF))
>         result += line;
> 
>    const SBuf all = tk.remaining();
> 
>    const auto crLen = tk.skipOne(CR); // may not be there
>    if (tk.skipOne(LF)) {
>      if (tk.skipAll(WSP)) // obs-fold!
>        result += ' '; // replace one obs-fold with one SP
>      else // CRLF
>        result += Http1::CrLF();
> 
>    } else if (crLen) {
>        result += all.substr(0, crLen);
>    }
>  }
> 

- too complex
- two expensive operations (+=) per iteration
- replaces bare LF with CRLF [while lying that it does not].
* still looks very different from the initial proposal IMO.


> + * \param containsObsFold will be set to true if obs-fold pattern is found. Otherwise not changed.

Please fix the "otherwise" part: A reasonable API must always set
containsObsFold. It is trivial (and not expensive) to clear it at the
beginning of the function, of course.

Please feel free to commit when you are comfortable with the results.

Alex.