Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Michael Chastain <mec.gnu@mindspring.com>
To: gdb-patches@sources.redhat.com, baurzhan.ismagulov@sbs.com.tr
Subject: Re: testcase for "absolute source" patch
Date: Mon, 16 Aug 2004 19:15:00 -0000	[thread overview]
Message-ID: <412107B7.nailE7I1XJVIH@mindspring.com> (raw)
In-Reply-To: <20040816144349.GB1509@ata.cs.hun.edu.tr>

Okay, here's a bunch of feedback on superficial stuff.
I haven't gotten to the substantive level of the test yet,
but it looks very useful.

  # Please email any bugs, comments, and/or additions to this file to:
  # bug-gdb@prep.ai.mit.edu

    This e-mail address is obsolete, so drop this.

  # XXX: We assume that build == host.

    Not good.  You have to at least try to work in a build != host
    environment.

    gdb_compile already downloads the srcfile from build to host.
    You can do "remote_download host" for more files, and
    "remote_exec host" to make directories and stuff.

  Style notes:

    Put a comment before each proc.  It can be as short as one line.

    Set the indentation level to 4 spaces.
    But tabs are still always eight spaces.
      (shiftwidth=4 tabstob=8 in vi)
      (i don't know the emacs settings)

    Check for errors on every system call, including cd.
    See lib/gdb.exp proc gdb_get_line_number for one way to do this;

      if { [ catch { cd ... } message ] } then {
	perror "$message"
	return -1
      }

    You don't need as many quotes and braces as you've been using.
    This works just fine:

      if { [gdb_compile $src $bin executable \

    It works even if $src or $bin have magic characters or spaces
    in them.  Unlike shell syntax, Tcl figures out the word boundaries
    first and then expands $src .  So $src always expands to exactly
    one word.

    If you look at gdb_get_line_number again, you will see that I
    did not know that either when I wrote it!

    Please don't name your variable "tmp".  Conceptually it's not
    even temporary; its value is the permanent directory that you
    keep coming back to.  How about "old_pwd" or "dir_current"
    or "oldpwd" or just about anything but "tmp".

  system

    Don't use system; use "remote_exec build ..." to run on the
    build machine or "remote_exec host ..." to run on the host
    machine.  You usually want "remote_exec host ...".

  echo

    Change this so that openp.c is a real file.  This makes it
    easier to find things.

    Also "main(){}" is not a legal Ansi C program.  Change it to "int
    main(){return 0;}" at least.  You can arrange the spacing any way
    you want -- it's explicitly encouraged to have a diversity of
    program formatting in the test programs.  But make it Ansi C.  Then
    you can drop the "-w" flag in gdb_compile.  Then I won't have
    problems later with hpux ansi c or ibm aix xlc.

    It's cool to have a one-line C program.  It's a trivial program
    so it does not need a copyright notice or license.

  Don't bother removing testtmpdir at the end unless it's really
  large, and it's nowhere near that large.  If somebody is
  investigating what happened with your test, they will want this
  directory to exist.


  parent reply	other threads:[~2004-08-16 19:15 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2004-08-16 14:41 Baurzhan Ismagulov
2004-08-16 15:56 ` Michael Chastain
2004-08-16 18:21   ` Baurjan Ismagulov
2004-08-16 19:15 ` Michael Chastain [this message]
2004-08-16 20:38   ` Andreas Schwab
2004-08-18 13:03   ` Baurzhan Ismagulov
2004-08-18 15:31     ` Michael Chastain
2004-08-18 15:50       ` Baurzhan Ismagulov
2004-08-18 17:10         ` Michael Chastain
2004-08-18 19:00         ` Michael Chastain
2004-08-18 22:03           ` Baurjan Ismagulov
2004-08-18 22:46             ` Michael Chastain
2004-08-18 23:33             ` Michael Chastain
2004-08-19  4:03               ` Eli Zaretskii
2004-08-19  8:34                 ` Michael Chastain
2004-08-19  8:56               ` Michael Chastain
2004-08-19  9:37                 ` Baurjan Ismagulov
2004-08-19  9:34               ` Baurjan Ismagulov
2004-08-19  9:55                 ` Michael Chastain
2004-08-19 10:10                   ` Baurjan Ismagulov
2004-08-19 10:20                     ` Michael Chastain
2004-08-26 20:36                   ` Baurjan Ismagulov
2004-08-26 20:52                     ` Michael Chastain
2004-08-26 20:32               ` Baurjan Ismagulov
2004-08-27 14:16                 ` Michael Chastain
2004-08-27 16:45                   ` Baurjan Ismagulov
2004-08-29 11:56                     ` Michael Chastain
2004-08-31 15:01                     ` Michael Chastain
2004-09-25 21:12                       ` Baurjan Ismagulov
2004-08-18 15:50       ` Daniel Jacobowitz
2004-08-18 15:56         ` Baurjan Ismagulov
2004-08-18 16:04           ` Andreas Schwab

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=412107B7.nailE7I1XJVIH@mindspring.com \
    --to=mec.gnu@mindspring.com \
    --cc=baurzhan.ismagulov@sbs.com.tr \
    --cc=gdb-patches@sources.redhat.com \
    /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