You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by Curt Arnold <ca...@apache.org> on 2005/02/18 15:39:05 UTC
PatternParser's map of PatternConverters
I was porting this cover over to log4cxx and I'm am curious why the
value members of the maps in PatternParser are instances of String and
not Class or Constructor?
The initialization code for the static instances looks like:
globalRulesRegistry.put("c",
LoggerPatternConverter.class.getName());
so it isn't like the classes aren't already loaded. The use of String
would seem to add an unnecessary detour through class loader hell since
later the class name would need to be converted back to a class
instance for use.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: PatternParser's map of PatternConverters
Posted by Curt Arnold <ca...@apache.org>.
On Feb 18, 2005, at 9:44 AM, Ceki Gülcü wrote:
> At 04:20 PM 2/18/2005, Curt Arnold wrote:
>
>
>> Did you intend to make globalRulesRegistry visible outside the class?
>
> Probably not but I am not 100% sure. In the near future, subclasses of
> PatternLayout might want to deal with formatting web server logs with
> quite different set of data.
If they override createClass() they could ignore the existing
globalRulesRegistry if they wanted.
>
>> It would seem to be a bad thing to have somebody else add entries to
>> that map after construction?
>
> Agreed. Once initialized, the global registry should become immutable.
Should be sufficient to make it private and only do read operations on
it. As long as it is not private, there is the potential for another
class to break that contract.
>
>> If that map is made private, the only place that accesses it is
>> findConverterClass which could be changed to either
>>
>> Class findConverterClass(String converterId)
>>
>> or
>>
>> PatternConverter createConverter(String converterId)
>>
>> which could encapsulate the different treatment of the global rules
>> from the instance rules.
>
> Sounds good. However, I suspect this part of the code to change again
> in the near future.
>
I think the createConverter is preferable.
Also, the error message in finalizeConverter looks like a hold-over
from the previous implementation. In testBogusWord1 and 2 which
attempt to parse "%, foobar" and "%xyz, foobar" respectively, the error
is "Unexpected char [,] at position N in conversion pattern" when the
real problem is that "" isn't a recognized format specifier. If you
had parsed "%foobar", the warning would have been "Unexpected char
[o]...".
I'll finish up the log4cxx's implementation and then update log4j's.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: PatternParser's map of PatternConverters
Posted by Ceki Gülcü <ce...@qos.ch>.
At 04:20 PM 2/18/2005, Curt Arnold wrote:
>Did you intend to make globalRulesRegistry visible outside the class?
Probably not but I am not 100% sure. In the near future, subclasses of
PatternLayout might want to deal with formatting web server logs with quite
different set of data.
>It would seem to be a bad thing to have somebody else add entries to that
>map after construction?
Agreed. Once initialized, the global registry should become immutable.
>If that map is made private, the only place that accesses it is
>findConverterClass which could be changed to either
>
>Class findConverterClass(String converterId)
>
>or
>
>PatternConverter createConverter(String converterId)
>
>which could encapsulate the different treatment of the global rules from
>the instance rules.
Sounds good. However, I suspect this part of the code to change again in
the near future.
--
Ceki Gülcü
The complete log4j manual: http://www.qos.ch/log4j/
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: PatternParser's map of PatternConverters
Posted by Curt Arnold <ca...@apache.org>.
On Feb 18, 2005, at 8:52 AM, Ceki Gülcü wrote:
> At 03:39 PM 2/18/2005, Curt Arnold wrote:
>
> Well, the idea is to have a single pattern for finding the converter
> to use. The converters in the global registry can be classes as they
> are already known at compile time. However, those in the converter
> registry come from data in string format originating in config files.
>
> Make sense? Can you think of anything better?
>
I hadn't considered PatternConverter class names being specified in the
configuration file. PatternConverter's are currently traditional C++
classes in log4cxx so they don't have all the java.lang.Class mimicry
that allows lookup by class name.
Did you intend to make globalRulesRegistry visible outside the class?
It would seem to be a bad thing to have somebody else add entries to
that map after construction?
If that map is made private, the only place that accesses it is
findConverterClass which could be changed to either
Class findConverterClass(String converterId)
or
PatternConverter createConverter(String converterId)
which could encapsulate the different treatment of the global rules
from the instance rules.
>
> ps: I started with class values but had to change once it became clear
> that data would be specified in config files.
>
Just retracing your steps.
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org
Re: PatternParser's map of PatternConverters
Posted by Ceki Gülcü <ce...@qos.ch>.
At 03:39 PM 2/18/2005, Curt Arnold wrote:
> I was porting this cover over to log4cxx and I'm am curious why the
> value members of the maps in PatternParser are instances of String and
> not Class or Constructor?
>
>The initialization code for the static instances looks like:
>
> globalRulesRegistry.put("c", LoggerPatternConverter.class.getName());
>
>so it isn't like the classes aren't already loaded. The use of String
>would seem to add an unnecessary detour through class loader hell since
>later the class name would need to be converted back to a class instance
>for use.
Well, the idea is to have a single pattern for finding the converter to
use. The converters in the global registry can be classes as they are
already known at compile time. However, those in the converter registry
come from data in string format originating in config files.
Make sense? Can you think of anything better?
ps: I started with class values but had to change once it became clear that
data would be specified in config files.
--
Ceki Gülcü
The complete log4j manual: http://www.qos.ch/log4j/
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org