[m-rev.] for review: source-linking for mdb

[Mark Brown]

On 27-Oct-2001, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> On 26-Oct-2001, Mark Brown <dougl at cs.mu.OZ.AU> wrote:
> > New commands added to mdb are 'source_window', which
> > opens a source-linked window if there is not one already open, and
> > 'source_window_close' which shuts any open source window.
> 
> Those command names are long and ugly,
> which is not good for interactive use.
> Something more concise would be much better.
> E.g. how about `edit' and `edit -c' (or `edit --close')?

I avoided that name for the reason David mentioned.  If the user edits
the file, it will be out of sync with the executable and its line
numbers.  'mdb' doesn't check the datestamp on a source file, so it can't
warn the user when this has happened.  So I thought a good compromise
would be to open the file read-only, so that users will get a warning
from vim if they try to edit it.

Note that experts can ignore the warning and edit the file anyway.  But
they will have to take responsibility for recompiling and rerunning the
executable themselves.

> 
> Also IMHO there should be a single-character alias for the
> first command, e.g. `alias e edit'.

Probably 'w' would be the best alias.  But mdb has other types of
windows too (such as an output window) so perhaps 'sw' would be better?
Anyone like to make any other suggestions?

> 
> > +		}
> > +		else if (word_count != 1)
> > +		{
> 
> The } should be on the same line as the else
> and the { should be on the same line as the if.
> 
> Likewise through-out this diff.

Done.

> 
> > +static const char *
> > +MR_trace_new_source_window(const char *window_cmd, const char *server_cmd,
> > +		const char *server_name, int timeout, bool force)
> 
> It would help to document what that function does.
> 
> > +static	void
> > +MR_trace_maybe_sync_source_window(MR_Event_Info *event_info)
> 
> Likewise.

Done.

> 
> > +	const MR_Label_Layout	*parent;
> > +	const char		*filename;
> > +	int			lineno;
> > +	const char		*problem; /* not used */
> > +	MR_Word			*base_sp, *base_curfr;
> > +	const char		*msg;
> > +
> > +	if (MR_trace_source_server.server_name != NULL) {
> > +		lineno = 0;
> > +		filename = "";
> > +
> > +		/*
> > +		** At interface ports we use the parent context if we can.
> > +		** Otherwise, we use the current context.
> > +		*/
> > +		if (MR_port_is_interface(event_info->MR_trace_port)) {
> > +			base_sp = MR_saved_sp(event_info->MR_saved_regs);
> > +			base_curfr = MR_saved_curfr(event_info->MR_saved_regs);
> > +			parent = MR_find_nth_ancestor(event_info->MR_event_sll,
> > +				1, &base_sp, &base_curfr, &problem);
> > +			if (parent != NULL) {
> > +				(void) MR_find_context(parent, &filename,
> > +					&lineno);
> > +			}
> > +		}
> > +
> > +		if (filename[0] == '\000') {
> 
> s/000/0/

Done.

> 
> > +	/*
> > +	** Start the server in read-only mode, to discourage the user
> > +	** from trying to edit the source.
> > +	*/
> > +	sprintf(system_call, "%s %s -R --servername \"%s\" &",
> > +			real_window_cmd, real_server_cmd, name);
> 
> I think it might be nicer to make `-R' part of the default
> server name ("vim -R"), so that the user can turn that off
> if they want.

Possibly.  Bear in mind that the same command is used for the client,
which would not need the '-R' (although it wouldn't hurt).  If we
really want to give the user the option of modifying the source without
getting a vim warning, then I think it would be better to provide an
option to the 'source_window' command to do that.  E.g. '-R' or
'--read-only' (which would be the default) would open vim read-only,
and '-R-' or '--no-read-only' would open it in normal mode.  How does
that sound?

> 
> > +	/*
> > +	** We first send "Ctrl-\ Ctrl-N", which guarantees we get back to
> > +	** "normal" mode without beeping,
> 
> How is that supposed to work?
> 
> It doesn't seem to work when I tried testing it.
> E.g. from insert mode, Ctrl-\ Ctrl-N had no effect.

It works for me.  Could a third person test this, please?

Cheers,
Mark.


diff -u trace/mercury_trace_internal.c trace/mercury_trace_internal.c
--- trace/mercury_trace_internal.c
+++ trace/mercury_trace_internal.c
@@ -1165,12 +1165,9 @@
 		{
 			; /* the usage message has already been printed */
 		}
-		else if (word_count != 1)
-		{
+		else if (word_count != 1) {
 			MR_trace_usage("browsing", "source_window");
-		}
-		else
-		{
+		} else {
 			msg = MR_trace_new_source_window(window_cmd,
 					server_cmd, server_name, timeout,
 					force);
@@ -1184,12 +1181,9 @@
 	} else if (streq(words[0], "source_window_close")) {
 		const char		*msg;
 
-		if (word_count != 1)
-		{
+		if (word_count != 1) {
 			MR_trace_usage("browsing", "source_window_close");
-		}
-		else
-		{
+		} else {
 			MR_trace_maybe_close_source_window();
 		}
 	} else if (streq(words[0], "break")) {
@@ -2385,6 +2379,14 @@
 	return FALSE;
 }
 
+/*
+** Implement the `source_window' command.  First, check if there is a
+** server attached.  If so, either stop it or abort the command, depending
+** on whether '-f' was given.  Then, if a server name was not supplied,
+** start a new server with a unique name (which has been MR_malloc'd),
+** otherwise attach to the server with the supplied name (and make a
+** MR_malloc'd copy of the name).
+*/
 static const char *
 MR_trace_new_source_window(const char *window_cmd, const char *server_cmd,
 		const char *server_name, int timeout, bool force)
@@ -2426,6 +2428,10 @@
 	return msg;
 }
 
+/*
+** If we are attached to a source server, then find the appropriate
+** context and ask the server to point to it, otherwise do nothing.
+*/
 static	void
 MR_trace_maybe_sync_source_window(MR_Event_Info *event_info)
 {
@@ -2455,7 +2461,7 @@
 			}
 		}
 
-		if (filename[0] == '\000') {
+		if (filename[0] == '\0') {
 			(void) MR_find_context(event_info->MR_event_sll,
 					&filename, &lineno);
 		}
@@ -2469,6 +2475,9 @@
 	}
 }
 
+/*
+** Close a source server, if there is one attached.
+*/
 static	void
 MR_trace_maybe_close_source_window(void)
 {
@@ -2975,8 +2984,7 @@
 				break;
 
 			case 't':
-				if (sscanf(MR_optarg, "%d", timeout) != 1)
-				{
+				if (sscanf(MR_optarg, "%d", timeout) != 1) {
 					MR_trace_usage(cat, item);
 					return FALSE;
 				}
diff -u trace/mercury_trace_source.c trace/mercury_trace_source.c
--- trace/mercury_trace_source.c
+++ trace/mercury_trace_source.c
@@ -73,11 +73,9 @@
 	/*
 	** Try running the server with '--version', and see if the
 	** output contains the string '+clientserver'.
-	**
-	** XXX if either server_cmd or fgrep is not found, the shell
-	** will print an error message that will end up on our terminal.
 	*/
-	sprintf(system_call, "%s --version 2>&1 | fgrep -q '+clientserver'",
+	sprintf(system_call,
+	    "%s --version 2>&1 | fgrep -q '+clientserver' > /dev/null 2>&1",
 		server_cmd);
 	status = system(system_call);
 	if (status == 0) {
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------