* Re: [SCM] master: Refactor Log to Callers. [not found] <20080213000530.24916.qmail@sourceware.org> @ 2008-02-14 10:02 ` Mark Wielaard 2008-02-27 12:03 ` Mark Wielaard 0 siblings, 1 reply; 3+ messages in thread From: Mark Wielaard @ 2008-02-14 10:02 UTC (permalink / raw) To: frysk Hi Andrew, On Wed, 2008-02-13 at 00:05 +0000, cagney@sourceware.org wrote: > Refactor Log to Callers. > > frysk-core/frysk/proc/dead/ChangeLog > 2008-02-12 Andrew Cagney <cagney@redhat.com> > > * LinuxCoreProc.java: Use Log.CALLER. > * LinuxCoreTask.java: Ditto. > > frysk-sys/frysk/rsl/ChangeLog > 2008-02-12 Andrew Cagney <cagney@redhat.com> > > * Node.java (set(Level)): Return the node. > * Callers.java: Extract from Log.java. > * TestCallers.java: New. > * Log.java (CALLER): New. > (CALLERS): New. > (dump(Object)): Check for null. > (set(PrintWriter)): New. > (set(PrintStream)): Return old writer. If you refactor working code could you please explain why. The functionality does seem almost the same now, but it is far more complex to see why it works. Depending on Callers.toString() in Log.dump() seems somewhat fragile unless you fix the call depth that method is called from somehow. There should at least be a warning in Log.java to not nest log() and dump() calls that might take a CALLER or CALLERS. Also the original code had comments for each method and field explaining why it was necessary, what it did and how to use it. It would be nice if the new code had the same (why is the method Callers.callers() included for example?) Thanks, Mark ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [SCM] master: Refactor Log to Callers. 2008-02-14 10:02 ` [SCM] master: Refactor Log to Callers Mark Wielaard @ 2008-02-27 12:03 ` Mark Wielaard 2008-03-10 11:28 ` Mark Wielaard 0 siblings, 1 reply; 3+ messages in thread From: Mark Wielaard @ 2008-02-27 12:03 UTC (permalink / raw) To: frysk; +Cc: Andrew Cagney Hi Andrew, On Thu, 2008-02-14 at 11:01 +0100, Mark Wielaard wrote: > On Wed, 2008-02-13 at 00:05 +0000, cagney@sourceware.org wrote: > > Refactor Log to Callers. > > > > frysk-core/frysk/proc/dead/ChangeLog > > 2008-02-12 Andrew Cagney <cagney@redhat.com> > > > > * LinuxCoreProc.java: Use Log.CALLER. > > * LinuxCoreTask.java: Ditto. > > > > frysk-sys/frysk/rsl/ChangeLog > > 2008-02-12 Andrew Cagney <cagney@redhat.com> > > > > * Node.java (set(Level)): Return the node. > > * Callers.java: Extract from Log.java. > > * TestCallers.java: New. > > * Log.java (CALLER): New. > > (CALLERS): New. > > (dump(Object)): Check for null. > > (set(PrintWriter)): New. > > (set(PrintStream)): Return old writer. > > If you refactor working code could you please explain why. > The functionality does seem almost the same now, but it is far more > complex to see why it works. Depending on Callers.toString() in > Log.dump() seems somewhat fragile unless you fix the call depth that > method is called from somehow. There should at least be a warning in > Log.java to not nest log() and dump() calls that might take a CALLER or > CALLERS. Also the original code had comments for each method and field > explaining why it was necessary, what it did and how to use it. It would > be nice if the new code had the same (why is the method > Callers.callers() included for example?) I see you refactored the logging code some more, but didn't add any documentation about the above. It would be good to have some documentation discussing the above in the code so others can help maintain it later on. Thanks, Mark ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [SCM] master: Refactor Log to Callers. 2008-02-27 12:03 ` Mark Wielaard @ 2008-03-10 11:28 ` Mark Wielaard 0 siblings, 0 replies; 3+ messages in thread From: Mark Wielaard @ 2008-03-10 11:28 UTC (permalink / raw) To: frysk; +Cc: Andrew Cagney Hi Andrew, On Wed, 2008-02-27 at 13:03 +0100, Mark Wielaard wrote: > On Thu, 2008-02-14 at 11:01 +0100, Mark Wielaard wrote: > > On Wed, 2008-02-13 at 00:05 +0000, cagney@sourceware.org wrote: > > > Refactor Log to Callers. > > > > > > frysk-core/frysk/proc/dead/ChangeLog > > > 2008-02-12 Andrew Cagney <cagney@redhat.com> > > > > > > * LinuxCoreProc.java: Use Log.CALLER. > > > * LinuxCoreTask.java: Ditto. > > > > > > frysk-sys/frysk/rsl/ChangeLog > > > 2008-02-12 Andrew Cagney <cagney@redhat.com> > > > > > > * Node.java (set(Level)): Return the node. > > > * Callers.java: Extract from Log.java. > > > * TestCallers.java: New. > > > * Log.java (CALLER): New. > > > (CALLERS): New. > > > (dump(Object)): Check for null. > > > (set(PrintWriter)): New. > > > (set(PrintStream)): Return old writer. > > > > If you refactor working code could you please explain why. > > The functionality does seem almost the same now, but it is far more > > complex to see why it works. Depending on Callers.toString() in > > Log.dump() seems somewhat fragile unless you fix the call depth that > > method is called from somehow. There should at least be a warning in > > Log.java to not nest log() and dump() calls that might take a CALLER or > > CALLERS. Also the original code had comments for each method and field > > explaining why it was necessary, what it did and how to use it. It would > > be nice if the new code had the same (why is the method > > Callers.callers() included for example?) > > I see you refactored the logging code some more, but didn't add any > documentation about the above. It would be good to have some > documentation discussing the above in the code so others can help > maintain it later on. Ping. Please do add some documentation here. It really helps. Thanks, Mark ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-03-10 11:28 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20080213000530.24916.qmail@sourceware.org> 2008-02-14 10:02 ` [SCM] master: Refactor Log to Callers Mark Wielaard 2008-02-27 12:03 ` Mark Wielaard 2008-03-10 11:28 ` Mark Wielaard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).