digitalmars.D.dwt - DWT2 Phobos-compatible logging suggestion
- torhu (20/20) Jul 13 2010 Ok, this is sort my way to get some cooperation going on the DWT
- Jeremy Powers (41/63) Jul 13 2010 Looks reasonable to me. Some comments:
- Jacob Carlborg (9/74) Jul 14 2010 I agree, if the method signatures are the same on both phobos and tango
- torhu (12/47) Jul 14 2010 You mean the prefixes printed in the Phobos version? They're probably
- Jacob Carlborg (6/26) Jul 14 2010 Seems good, just a question: what is this diff against? Is it to your
- torhu (2/33) Jul 14 2010 Yes, it's my own branch.
- torhu (6/6) Jul 14 2010 Second version of this patch, still based on my fork:
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.
Jul 13 2010
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.
=C2=A0Check 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, ...). =C2=A0Using 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. =C2=A0I just did fmt =3D replace(fmt,=
"{}",
"%s") in the Phobos version, which takes care of most cases. =C2=A0We 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 canno=
t
be virtual. Cfr. item 1. =C2=A0The 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.
Jul 13 2010
Jacob Carlborg <doob me.com> 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: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.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:-- Jacob CarlborgOk, 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.
Jul 14 2010
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:That'd be nice. I don't know if the Phobos stack tracing can be used like that, though.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?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.
Jul 14 2010
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
Jul 14 2010
On 14.07.2010 11:56, Jacob Carlborg wrote:On 2010-07-13 23.00, torhu wrote:Yes, it's my own branch.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.
Jul 14 2010
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.
Jul 14 2010