All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
	Gianni Tedesco <gianni.tedesco@citrix.com>,
	Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
Subject: Re: [PATCH] libxl: new xlu_disk_parse function
Date: Fri, 1 Apr 2011 16:28:27 +0100	[thread overview]
Message-ID: <alpine.DEB.2.00.1104011625350.16492@kaball-desktop> (raw)
In-Reply-To: <19860.47828.119869.764622@mariner.uk.xensource.com>

On Thu, 31 Mar 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function"):
> > On Mon, 28 Mar 2011, Gianni Tedesco wrote:
> > > Hmm, I'm not sure this is nicer than the code it's replacing, it's
> > > certainly a lot longer. I don't like the idea of it being full of things
> > > comparing for ",w" or ":cdrom" etc, rather than tokenising it fully and
> > > evaluating what the tokens are separately.
> > 
> > I definitely agree.
> > Besides when doing refactoring the code produced should be either shorter or
> > at least easier to read but this code is neither of them.
> 
> I agree that as a rule of thumb better code is shorter.  Unfortunately
> string parsing in C is one of those exceptions.
> 
> What I was trying to do was to separate out the actual syntax in a way
> that is reasonably tractable, and which is amenable to being edited
> later without introducing bugs.
> 
> So while the current code is longer in this case I think it's clearer.
> A good deal of the added code is very simple and straightforward "set
> up this regexp engine and check the return value".

The problem with this code is that, exactly like regex in general, is
write-only.
In one year time making a change to this code would be harder than
rewrite it again from scratch for the third time.
I vote for the explicit state machine.

  reply	other threads:[~2011-04-01 15:28 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-03-24 17:49 [PATCH] libxl: move TOSTRING to libxl_internal.h Ian Jackson
2011-03-24 17:49 ` [PATCH] libxl: new xlu_disk_parse function Ian Jackson
2011-03-24 17:49   ` [PATCH] xl: replace config file disk spec parser with call to xlu_disk_parse Ian Jackson
2011-03-24 17:49     ` [PATCH] xl: replace block-attach disk config parser with call to xlu_parse_disk Ian Jackson
2011-03-28 17:31   ` [PATCH] libxl: new xlu_disk_parse function Gianni Tedesco
2011-03-28 18:13     ` Stefano Stabellini
2011-03-29  9:10       ` Ian Campbell
2011-03-31 17:33       ` Ian Jackson
2011-04-01 15:28         ` Stefano Stabellini [this message]
2011-03-31 17:30     ` Ian Jackson
2011-04-02  8:51       ` Ian Campbell
2011-04-04 11:29       ` Stefano Stabellini
2011-04-14  7:20       ` Gianni Tedesco

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=alpine.DEB.2.00.1104011625350.16492@kaball-desktop \
    --to=stefano.stabellini@eu.citrix.com \
    --cc=Ian.Jackson@eu.citrix.com \
    --cc=gianni.tedesco@citrix.com \
    --cc=xen-devel@lists.xensource.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.