Ok, this is sort my way to get some cooperation going on the DWT project.  Check it out and make suggestions for improvements, or just say "ok" if you approve :)

I've been trying for a couple of hours to find a way to do logging that's compatible with both Phobos and Tango.

Here's what I came up with, this is tested and working:
http://pastebin.ca/1900029

This is what I had to take into consideration:

1) Phobos formatting uses functions like format(...), while tango uses format(char[] fmt, ...).  Using the signature func(T...)(String fmt, T args) was the simplest way I found to be able to just forward arguments to both kinds of formatters.

2) Format specifiers are different.  I just did fmt = replace(fmt, "{}", "%s") in the Phobos version, which takes care of most cases.  We can revisit this later if there's an actual need for more flexible formatting.

3) The IDwtLogger interface had to go, as template member functions cannot be virtual. Cfr. item 1.  The interface was not put to use anywhere, so this should be of no concern.

4) ExceptionPrintStackTrace has become less flexible, but this shouldn't matter and can easily be fixed if needed.

Looks reasonable to me.  Some comments:

I see a danger w/ having the whole class version()'ed around - the methods are defined twice, and need to be kept in sync.  Would it be better to just do the version around the implementations?  I.E. something like:

  class DWTLogger {
    version(Tango) {
      tango.util.log.Log.Logger logger;
    }
    ...
    void info(T...)(String file, ulong line, String fmt, T args){
      version(Tango) {
        ...
      } else {
        ...
      }
    }

Also, should the tags for the different levels match log4j style? That is, use 'INFO', 'DEBUG', etc....

Aside:  I was hoping there would be a D-standard logging solution similar to log4j, but haven't found one outside of Tango yet - not sure how important logging is to DWT itself, but this is definitely something needed for many projects.  If such a thing did exist, we could just delegate to it (or use directly).

One more:

> 4) ExceptionPrintStackTrace has become less flexible, but this shouldn't matter and can easily be fixed if needed.

I've been mucking around with the way exceptions are defined, to allow Java-translated code to work with less changes, so assuming I actually get that done and it works the ExceptionPrintStackTrace method should be unneeded - in favor of just doing e.printStackTrace().  Anyway, when I get that part finished I'll send something to the list.


I'm actually a big fan of code reviews as a form of collaboration - do you want to use the list as a place for this kind of thing, or does it make sense to do something more formal w/ issue tracking etc?

Jeremy

On Tue, Jul 13, 2010 at 2:00 PM, torhu <no@spam.invalid> wrote:
> Ok, this is sort my way to get some cooperation going on the DWT project.  Check it out and make suggestions for improvements, or just say "ok" if you approve :)
>
> I've been trying for a couple of hours to find a way to do logging that's compatible with both Phobos and Tango.
>
> Here's what I came up with, this is tested and working: http://pastebin.ca/1900029
>
> This is what I had to take into consideration:
>
> 1) Phobos formatting uses functions like format(...), while tango uses
> format(char[] fmt, ...).  Using the signature func(T...)(String fmt, T args)
> was the simplest way I found to be able to just forward arguments to both
> kinds of formatters.
>
> 2) Format specifiers are different.  I just did fmt = replace(fmt, "{}", "%s") in the Phobos version, which takes care of most cases.  We can revisit this later if there's an actual need for more flexible formatting.
>
> 3) The IDwtLogger interface had to go, as template member functions cannot be virtual. Cfr. item 1.  The interface was not put to use anywhere, so this should be of no concern.
>
> 4) ExceptionPrintStackTrace has become less flexible, but this shouldn't matter and can easily be fixed if needed.
>

On 2010-07-13 23.00, torhu wrote:
> Ok, this is sort my way to get some cooperation going on the DWT
> project.  Check it out and make suggestions for improvements, or just
> say "ok" if you approve :)
>
> I've been trying for a couple of hours to find a way to do logging
> that's compatible with both Phobos and Tango.
>
> Here's what I came up with, this is tested and working:
> http://pastebin.ca/1900029
>
> This is what I had to take into consideration:
>
> 1) Phobos formatting uses functions like format(...), while tango uses
> format(char[] fmt, ...). Using the signature func(T...)(String fmt, T
> args) was the simplest way I found to be able to just forward arguments
> to both kinds of formatters.
>
> 2) Format specifiers are different. I just did fmt = replace(fmt, "{}",
> "%s") in the Phobos version, which takes care of most cases. We can
> revisit this later if there's an actual need for more flexible formatting.
>
> 3) The IDwtLogger interface had to go, as template member functions
> cannot be virtual. Cfr. item 1. The interface was not put to use
> anywhere, so this should be of no concern.
>
> 4) ExceptionPrintStackTrace has become less flexible, but this shouldn't
> matter and can easily be fixed if needed.

Seems good, just a question: what is this diff against? Is it to your own branch or to the DWT2 main branch? I don't see a version condition in ExceptionPrintStackTrace in the DWT2 main branch for example.

-- 
Jacob Carlborg

On 2010-07-13 23.46, Jeremy Powers wrote:
> Looks reasonable to me.  Some comments:
>
> I see a danger w/ having the whole class version()'ed around - the
> methods are defined twice, and need to be kept in sync.  Would it be
> better to just do the version around the implementations?  I.E.
> something like:
>
>    class DWTLogger {
>      version(Tango) {
>        tango.util.log.Log.Logger logger;
>      }
>      ...
>      void info(T...)(String file, ulong line, String fmt, T args){
>        version(Tango) {
>          ...
>        } else {
>          ...
>        }
>      }

I agree, if the method signatures are the same on both phobos and tango then the version condition should be located inside the method.

> Also, should the tags for the different levels match log4j style?
> That is, use 'INFO', 'DEBUG', etc....
>
> Aside:  I was hoping there would be a D-standard logging solution
> similar to log4j, but haven't found one outside of Tango yet - not
> sure how important logging is to DWT itself, but this is definitely
> something needed for many projects.  If such a thing did exist, we
> could just delegate to it (or use directly).
>
> One more:
>
>> 4) ExceptionPrintStackTrace has become less flexible, but this shouldn't
>> matter and can easily be fixed if needed.
>
> I've been mucking around with the way exceptions are defined, to allow
> Java-translated code to work with less changes, so assuming I actually
> get that done and it works the ExceptionPrintStackTrace method should
> be unneeded - in favor of just doing e.printStackTrace().  Anyway,
> when I get that part finished I'll send something to the list.
>
>
> I'm actually a big fan of code reviews as a form of collaboration - do
> you want to use the list as a place for this kind of thing, or does it
> make sense to do something more formal w/ issue tracking etc?

Hm, I think using the issue tracking system might be better, it easier to keep track of and it doesn't get lost so easy. We can use the issue tracking system for the DWT project, the DWT2 project doesn't have a trac environment.

> Jeremy
>
> On Tue, Jul 13, 2010 at 2:00 PM, torhu<no@spam.invalid>  wrote:
>> Ok, this is sort my way to get some cooperation going on the DWT project.
>>   Check it out and make suggestions for improvements, or just say "ok" if you
>> approve :)
>>
>> I've been trying for a couple of hours to find a way to do logging that's
>> compatible with both Phobos and Tango.
>>
>> Here's what I came up with, this is tested and working:
>> http://pastebin.ca/1900029
>>
>> This is what I had to take into consideration:
>>
>> 1) Phobos formatting uses functions like format(...), while tango uses
>> format(char[] fmt, ...).  Using the signature func(T...)(String fmt, T args)
>> was the simplest way I found to be able to just forward arguments to both
>> kinds of formatters.
>>
>> 2) Format specifiers are different.  I just did fmt = replace(fmt, "{}",
>> "%s") in the Phobos version, which takes care of most cases.  We can revisit
>> this later if there's an actual need for more flexible formatting.
>>
>> 3) The IDwtLogger interface had to go, as template member functions cannot
>> be virtual. Cfr. item 1.  The interface was not put to use anywhere, so this
>> should be of no concern.
>>
>> 4) ExceptionPrintStackTrace has become less flexible, but this shouldn't
>> matter and can easily be fixed if needed.
>>


-- 
Jacob Carlborg

On 14.07.2010 11:56, Jacob Carlborg wrote:
> On 2010-07-13 23.00, torhu wrote:
>>  Ok, this is sort my way to get some cooperation going on the DWT
>>  project.  Check it out and make suggestions for improvements, or just
>>  say "ok" if you approve :)
>>
>>  I've been trying for a couple of hours to find a way to do logging
>>  that's compatible with both Phobos and Tango.
>>
>>  Here's what I came up with, this is tested and working:
>>  http://pastebin.ca/1900029
>>
>>  This is what I had to take into consideration:
>>
>>  1) Phobos formatting uses functions like format(...), while tango uses
>>  format(char[] fmt, ...). Using the signature func(T...)(String fmt, T
>>  args) was the simplest way I found to be able to just forward arguments
>>  to both kinds of formatters.
>>
>>  2) Format specifiers are different. I just did fmt = replace(fmt, "{}",
>>  "%s") in the Phobos version, which takes care of most cases. We can
>>  revisit this later if there's an actual need for more flexible formatting.
>>
>>  3) The IDwtLogger interface had to go, as template member functions
>>  cannot be virtual. Cfr. item 1. The interface was not put to use
>>  anywhere, so this should be of no concern.
>>
>>  4) ExceptionPrintStackTrace has become less flexible, but this shouldn't
>>  matter and can easily be fixed if needed.
>
> Seems good, just a question: what is this diff against? Is it to your
> own branch or to the DWT2 main branch? I don't see a version condition
> in ExceptionPrintStackTrace in the DWT2 main branch for example.
>

Yes, it's my own branch.

On 13.07.2010 23:46, Jeremy Powers wrote:
> Looks reasonable to me.  Some comments:
>
> I see a danger w/ having the whole class version()'ed around - the
> methods are defined twice, and need to be kept in sync.  Would it be
> better to just do the version around the implementations?  I.E.
> something like:
>
>    class DWTLogger {
>      version(Tango) {
>        tango.util.log.Log.Logger logger;
>      }
>      ...
>      void info(T...)(String file, ulong line, String fmt, T args){
>        version(Tango) {
>          ...
>        } else {
>          ...
>        }
>      }

Yes, you are right. I've done it that way now.

>
> Also, should the tags for the different levels match log4j style?
> That is, use 'INFO', 'DEBUG', etc....

You mean the prefixes printed in the Phobos version?  They're probably shortened to three letters to make the start of the content of each line align.  I think it's ok the way it is.


>
> Aside:  I was hoping there would be a D-standard logging solution
> similar to log4j, but haven't found one outside of Tango yet - not
> sure how important logging is to DWT itself, but this is definitely
> something needed for many projects.  If such a thing did exist, we
> could just delegate to it (or use directly).

The loggging in DWT is just for debugging, it doesn't need to be very flexible.  If Phobos gets a logging package (it has been discussed), we can hook into that like we do with Tango.

>
> One more:
>
>>  4) ExceptionPrintStackTrace has become less flexible, but this shouldn't
>>  matter and can easily be fixed if needed.
>
> I've been mucking around with the way exceptions are defined, to allow
> Java-translated code to work with less changes, so assuming I actually
> get that done and it works the ExceptionPrintStackTrace method should
> be unneeded - in favor of just doing e.printStackTrace().  Anyway,
> when I get that part finished I'll send something to the list.

That'd be nice.  I don't know if the Phobos stack tracing can be used like that, though.

>
> I'm actually a big fan of code reviews as a form of collaboration - do
> you want to use the list as a place for this kind of thing, or does it
> make sense to do something more formal w/ issue tracking etc?

Thing like a list of which snippets work and which don't would make sense to have on a wiki, I suppose.  Maybe tied to a ticket.

Second version of this patch, still based on my fork:
http://pastebin.ca/1900478

version statements are inside the functions now.

I removed the heap allocation when formatting log messages in the Tango version, see the writeLog function.  It's possible that I went a bit overboard on that one, but at least it's as fast as it gets now.