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

[Amos Jeffries]

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.


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




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

+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.

Amos