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
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
