On 11/05/2011 05:13 AM, Amos Jeffries wrote:
> This updates the format parser and storage objects in the Format::
> namespace and separates some into separate files for SourceLayout
> project dependency reductions.
>
> Functionally it adds a registration API so components can register
> themselves an array of tokens in a namespace. Registering the arbitrary
> namespace "example" with some tokens ("a","b") will cause the parser to
> accept those tokens in a logging format like so: "%example::a %example::b".
>
> Due to time limitations resolving the library-library symbol
> perculiarities in GCC I have had to cut short of moving the "adapt::"
> and "icap::" tokens into their libraries. But have gone so far as to
> make them use the registration process in this patch ready for that step.
>
> Future work:
> - convert the error pages to use format for the page body macros
> - convert the %ssl_* tokens in src/ssl/* to use format and the
> namespace "ssl::"
> - convert external_acl_type to use formats for its helper input string.
>
> Amos
> + explicit TokenNamespace(const String &nsName, TokenTableEntry const *tSet) : prefix(nsName), tokenSet(tSet) {};
No "explicit" for constructors with more than one required parameter
because they cannot be activated implicitly.
> + ~TokenNamespace() {};
Extra semicolon.
Not needed as the default destructor is the same.
> + /// array of tokens inside this namespace
> + TokenTableEntry const *tokenSet;
AFAICT, 3rd-party code modifies this set. Should we prohibit copying and
assigning of TokenNamespace objects then?
> +class FmtConfig
> +{
> +public:
The new class needs a description, especially if we declare a global of
its type later:
> +extern FmtConfig TheConfig;
> +#define free_format(X) do{ delete (*X).formats; (*X).formats=NULL; }while(false)
Technically, you need parens around X above.
> - {"<st", LFT_ICAP_BYTES_READ},
> + {">st", LFT_ICAP_BYTES_SENT},
> + {"<st", LFT_ICAP_BYTES_READ},
> {"<bs", LFT_ICAP_BODY_BYTES_READ},
>
> {">h", LFT_ICAP_REQ_HEADER},
> {"<h", LFT_ICAP_REP_HEADER},
>
> {"tr", LFT_ICAP_TR_RESPONSE_TIME},
> - {"tio", LFT_ICAP_IO_TIME},
> + {"tio", LFT_ICAP_IO_TIME},
> {"to", LFT_ICAP_OUTCOME},
> {"Hs", LFT_ICAP_STATUS_CODE},
>
> - {NULL, LFT_NONE} /* this must be last */
> + {NULL, LFT_NONE} /* this must be last */
Whitespace non-changes?
> +class TokenTableEntry {
The new class needs a description. There is a broader description for
"namespace Format" above it, but we should define what this specific
class is for.
> + const char *config;
> + ByteCode_t token_type;
> + int options;
Please document these. They are probably obvious to you, but some of us
need a hand to know what yet another "config" really is in this context.
> === modified file 'src/main.cc'
> --- src/main.cc 2011-11-03 10:02:02 +0000
> +++ src/main.cc 2011-11-05 09:41:52 +0000
> @@ -63,6 +63,7 @@
> #include "event.h"
> #include "EventLoop.h"
> #include "ExternalACL.h"
> +#include "format/Token.h"
> #include "fs/Module.h"
> #include "PeerSelectState.h"
> #include "Store.h"
> @@ -1105,6 +1106,8 @@
> // TODO: pconn is a good candidate for new-style registration
> // PconnModule::GetInstance()->registerWithCacheManager();
> // moved to PconnModule::PconnModule()
> +
> + Format::Token::Init();
> }
>
> if (IamPrimaryProcess()) {
Consider removing this dependency or even Format::Token::Init() as such
by using RunnersRegistry API. I understand you may not have time for
that, but perhaps you can add a corresponding TODO to discourage more
code being added to Format::Token::Init()?
> === renamed file 'src/format/Tokens.h' => 'src/format/Token.h'
> --- src/format/Tokens.h 2011-10-13 17:05:25 +0000
> +++ src/format/Token.h 2011-11-05 08:38:12 +0000
> @@ -1,5 +1,7 @@
> -#ifndef _SQUID_FMT_TOKENS_H
> -#define _SQUID_FMT_TOKENS_H
> +#ifndef _SQUID_FORMAT_TOKEN_H
> +#define _SQUID_FORMAT_TOKEN_H
> +
> +#include "format/TokenTableEntry.h"
Can you avoid this dependency by pre-declaring class TokenTableEntry? It
looks like we are only using a pointer to TokenTableEntry in
src/format/Token.h, no?
The above are all minor polishing touches. I did not see any bugs, and I
cannot say I understand this code well enough to find them.
Thank you,
Alex.
Received on Mon Nov 07 2011 - 20:38:04 MST
This archive was generated by hypermail 2.2.0 : Wed Nov 16 2011 - 12:00:08 MST