[squid-dev] [PATCH] Note ACL substrings matching

[Christos Tsantilas]

On 12/14/2015 08:50 PM, Amos Jeffries wrote:
> On 15/12/2015 5:43 a.m., Alex Rousskov wrote:
>> On 12/14/2015 09:04 AM, Amos Jeffries wrote:
>>> Last queston:
>>> * why m?
>>
>> "m" is for "member" -- this is a list Membership test. If you insist on
>> any other character, we will use that instead. For example, I agree that
>> "-d" for "delimited" or "-l" for "list" also makes sense.
>>
>
> Thanks. That was what I was looking for.
>
>
> Moving on to the more detailed audit;
>
>
> in src/acl/Acl.cc:
>
> * ACLFlags::FlagsTokenizer::nextFlag()
>   - the else condition does not need brackets (up to you, just shorter code)
>   - the tokPos should use pre-increment.
>
> * ACLFlags::parseFlags()
>   - status member is used outside the switch statement, please use
> supported(flag) directly as the switch() parameter.
>   - if you keeping it as local variable, please use auto or const auto
> type instead of Status. That helps avoid implicit enum casting and
> enables safer compiler checks on the switch case values.
>
>
> in src/acl/Acl.h:
>
> * please remove extra whitespace in "{ }" on the new constructor.
>
> * supported()
>   - Status is not a boolean type, so the functinon cannot return "True".
>     this should perhapse reference the enum of states, and say which
> one(s) are returned on success case(s).
>   - also s/flag supported/flag is supported/ assuming you keep that part
> of the text.
>
> * parameterSupported(), parameter(), and isSet()
>   - replace "True" with doxygen "\return true"
>   - replace "Return" with "\return"
>   - also most of the FlagsTokenizer methods need matching changes
>
> * class FlagsTokenizer description
>   - please use the (A)BNF syntax:
> "
>    Support flag tokens in the form:
>      flag :=  '-' [A-Z|a-z]+ ['=' parameter ]
> "
>
> * pre-existing flags_ documentation has wrong grammar, please take the
> oportunity to fix that.
>   - it should say "The flags which are set"  (s/is/are)
>
>
> in src/acl/Note.cc:
>
> * ACLNoteStrategy::match()
>   - the request local can be reduced in scope:
> "
>     if (const auto request = checklist->request)
> "
>
> * ACLNoteStrategy::noteDelimiters()
>   - the use of static on a local which gets reset on every use is
> pointless. It might as well be a normal local variable.
>   - I think enact the TODO
>
>
> in src/acl/Note.h:
> * class pre-defines in alphabetical order please.
>
>
> in src/acl/NoteData.h:
> * #includes in alphabetical order please
>   - if that means more includes elsewhere so be it, they have to pass the
> .h unit tests anyway.
>
>
> in src/acl/StringData.cc:
> * please add an XXX note that ACLStringData::match(char const *toFind)
> now has a performance regression because SBuf(char*) data-copies.
>   - it will also need a doxygen "\deprecated use match(SBuf&)" note to
> start people using the better one.
>

All of the above addressed in the patch applied to trunk

>
> in src/cf.data.pre:
> * what does "produce an empty token" mean when matching? it cannot be
> matched? that needs stating clearly.

Actually this was not true. There are not empty tokens. Two sequential 
separators, eg ",," handled as one separator.
The  Parser::Tokenizer class is used to get sub-strings, which has this 
behaviour.
The related sentence removed from cf.data.pre documentation.

>
>
> Please commit the CharacterSet changes as as separate commit. There are
> no audit details found in there, so +1 on CharacterSet right now.

The CharacterSet changes are not needed anymore. The 
ACLNoteStrategy::noteDelimiters() fixes removed the need for this 
changes too.

>
> +0 on the rest. most of it is documentation and polish, though there are
> a few logic requests. Up to you whether you want to go through another
> round of audit or just commit with the changes. I dont think it will
> needs to unless you have any objections to the above.

I applied the final patch to trunk as r14465.

>
> Amos