public inbox for frysk@sourceware.org
 help / color / mirror / Atom feed
From: Mark Wielaard <mark@klomp.org>
To: frysk@sourceware.org
Subject: Re: [SCM]  master: Refactor Log to Callers.
Date: Thu, 14 Feb 2008 10:02:00 -0000	[thread overview]
Message-ID: <1202983298.3365.17.camel@dijkstra.wildebeest.org> (raw)
In-Reply-To: <20080213000530.24916.qmail@sourceware.org>

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

       reply	other threads:[~2008-02-14 10:02 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20080213000530.24916.qmail@sourceware.org>
2008-02-14 10:02 ` Mark Wielaard [this message]
2008-02-27 12:03   ` Mark Wielaard
2008-03-10 11:28     ` Mark Wielaard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1202983298.3365.17.camel@dijkstra.wildebeest.org \
    --to=mark@klomp.org \
    --cc=frysk@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).