Re: [MERGE] wordlist refactoring

Kinkie wrote:
> Hi all,
> this patch is an attempt at streamlining the wordlist implementation
> in 3.HEAD, by:
> - OO-izing it [1]
> - providing an unit-test for it
> - giving it a couple of convenience functions
> - documenting it
>
> It's still a work-in-progress, but in the end a wordlist would probably
> be better implemented by using a set of Strings anyways.
> It's however a step forward already, since it gives wordlist callers
> better c++ semantics than the current implementation in HEAD.
>
> The patch has been compile-tested, unit-tested, test-suite'd and run-tested.
>
> Please review and hopefully apply.
>
> Kinkie
>

NP: all I've done is read the patch and mention things that stand out.
Some of the below need fixing, others just explaining why.

1)

in src/Makefile.am
@@ -1188,6 +1188,7 @@
          tests/testURL \
+ tests/testWordlist
          @STORE_TESTS@

   a) Missing a '\' on that line.
   b) How did this pass build tests? particularly the new testbed ones.

2)
  if (authenticate) {
     delete authenticate;
     authenticate = NULL;
  }
is equivalent to:
   safe_delete(authenticate);
provided by the shared squid compat library headers.

same for: Config.Program.redirect
same for: dnsservers->cmdline
same for: err->ftp.server_msg
same for: p->cmdline and p->arguments in external_acl line 194 & 574

in fact everywhere you've gone delete. Whether you are using if()
already or not. (The non-use of if() before delete is unsafe anyway).

3)
There is a lot of 'new' allocation going on thats followed by 'delete'
at the end of the parent scope.
   ie RemovalPolicySettings member pointer, adaptation ServiceGroup
functions
On the face of the patch without digging into the usage code, they
appear to be one-use memory
Why are these not able to be plain non-pointer member or local variable now?

4)
  I'm sure the ACL Walkee's don't need wordlist**, now that you are
pre-allocating the wordlist.

5)
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2008-06-09 01:58:19 +0000
+++ src/client_side_request.cc 2008-08-09 16:13:04 +0000
@@ -666,7 +666,7 @@
      const char *url = http->uri;
      HttpRequest *request = http->request;
      HttpRequestMethod method = request->method;
- const wordlist *p = NULL;
+ const wordlist *p;

Why is this small piece of pointer-safety being removed? '*p' is not
guaranteed to be initialized correctly outside the defining function scope.

6)
=== modified file 'src/tests/stub_tools.cc'
--- src/tests/stub_tools.cc 2006-09-15 02:13:23 +0000
+++ src/tests/stub_tools.cc 2008-08-11 14:54:43 +0000
@@ -45,3 +45,4 @@
  {
      fatal ("Not implemented");
  }
+

can be reverted from the merge.

7)
  for the documentation you don't need all the: "\ingroup wordlist"
class methods are automatically grouped.

8) as mentioned the nastiness in wordlist old behavior. That should go
if possible during this cleanup. IIRC it was mainly the O(N+1)
insertion/append algorithm, a simple tail pointer should speed that up
to O(1).
Adrian may have other bits he noticed, or you might have seen others
already yourself.

Amos

-- 
Please use Squid 2.7.STABLE3 or 3.0.STABLE8