Libguestfs - Dec 2016 - Re: [PATCH 2/2] v2v: Fix ambiguous and probably incorrect pattern match (warning 57).

On Thursday, 8 December 2016 13:54:04 CET Richard W.M. Jones
wrote:
> See:
> http://caml.inria.fr/pub/docs/manual-ocaml/comp.html#ss%3Awarn57
> 
> I believe the code as written previously was incorrect.  However we
> are lucky because if neither clause matches then it will fall through
> to displaying an error message, allowing the user to correct the
> problem.
> ---
>  v2v/output_vdsm.ml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/v2v/output_vdsm.ml b/v2v/output_vdsm.ml
> index a78e3e6..fb7dd3c 100644
> --- a/v2v/output_vdsm.ml
> +++ b/v2v/output_vdsm.ml
> @@ -83,6 +83,9 @@ object
>        let fields = List.rev fields in      (* "UUID"
"data-center" ... *)
>        match fields with
>        | "" :: uuid :: rest                 (* handles trailing
"/" case *)
> +          when String.length uuid = 36 ->
> +        let mp = String.concat "/" (List.rev rest) in
> +        mp, uuid
>        | uuid :: rest
>            when String.length uuid = 36 ->
>          let mp = String.concat "/" (List.rev rest) in
Rather than duplicating the (admittely small) code block, what about
removing the slash before splitting, so there is no need for the
separate match case handling the empty part?

-- 
Pino Toscano

On Fri, Dec 09, 2016 at 10:32:42AM +0100, Pino Toscano
wrote:
> On Thursday, 8 December 2016 13:54:04 CET Richard W.M. Jones wrote:
> > See:
> > http://caml.inria.fr/pub/docs/manual-ocaml/comp.html#ss%3Awarn57
> > 
> > I believe the code as written previously was incorrect.  However we
> > are lucky because if neither clause matches then it will fall through
> > to displaying an error message, allowing the user to correct the
> > problem.
> > ---
> >  v2v/output_vdsm.ml | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/v2v/output_vdsm.ml b/v2v/output_vdsm.ml
> > index a78e3e6..fb7dd3c 100644
> > --- a/v2v/output_vdsm.ml
> > +++ b/v2v/output_vdsm.ml
> > @@ -83,6 +83,9 @@ object
> >        let fields = List.rev fields in      (* "UUID"
"data-center" ... *)
> >        match fields with
> >        | "" :: uuid :: rest                 (* handles
trailing "/" case *)
> > +          when String.length uuid = 36 ->
> > +        let mp = String.concat "/" (List.rev rest) in
> > +        mp, uuid
> >        | uuid :: rest
> >            when String.length uuid = 36 ->
> >          let mp = String.concat "/" (List.rev rest) in
> 
> Rather than duplicating the (admittely small) code block, what about
> removing the slash before splitting, so there is no need for the
> separate match case handling the empty part?
Even better is ...

diff --git a/v2v/output_vdsm.ml b/v2v/output_vdsm.ml
index a78e3e6..c3be84d 100644
--- a/v2v/output_vdsm.ml
+++ b/v2v/output_vdsm.ml
@@ -81,10 +81,9 @@ object
     let mp, uuid        let fields = String.nsplit "/" os in (* ...
"data-center" "UUID" *)
       let fields = List.rev fields in      (* "UUID"
"data-center" ... *)
+      let fields = dropwhile ((=) "") fields in
       match fields with
-      | "" :: uuid :: rest                 (* handles trailing
"/" case *)
-      | uuid :: rest
-          when String.length uuid = 36 ->
+      | uuid :: rest when String.length uuid = 36 ->
         let mp = String.concat "/" (List.rev rest) in
         mp, uuid
       | _ ->

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Friday, 9 December 2016 11:29:39 CET Richard W.M. Jones
wrote:
> On Fri, Dec 09, 2016 at 10:32:42AM +0100, Pino Toscano wrote:
> > On Thursday, 8 December 2016 13:54:04 CET Richard W.M. Jones wrote:
> > > See:
> > > http://caml.inria.fr/pub/docs/manual-ocaml/comp.html#ss%3Awarn57
> > > 
> > > I believe the code as written previously was incorrect.  However
we
> > > are lucky because if neither clause matches then it will fall
through
> > > to displaying an error message, allowing the user to correct the
> > > problem.
> > > ---
> > >  v2v/output_vdsm.ml | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/v2v/output_vdsm.ml b/v2v/output_vdsm.ml
> > > index a78e3e6..fb7dd3c 100644
> > > --- a/v2v/output_vdsm.ml
> > > +++ b/v2v/output_vdsm.ml
> > > @@ -83,6 +83,9 @@ object
> > >        let fields = List.rev fields in      (* "UUID"
"data-center" ... *)
> > >        match fields with
> > >        | "" :: uuid :: rest                 (* handles
trailing "/" case *)
> > > +          when String.length uuid = 36 ->
> > > +        let mp = String.concat "/" (List.rev rest) in
> > > +        mp, uuid
> > >        | uuid :: rest
> > >            when String.length uuid = 36 ->
> > >          let mp = String.concat "/" (List.rev rest) in
> > 
> > Rather than duplicating the (admittely small) code block, what about
> > removing the slash before splitting, so there is no need for the
> > separate match case handling the empty part?
> 
> Even better is ...
> 
> diff --git a/v2v/output_vdsm.ml b/v2v/output_vdsm.ml
> index a78e3e6..c3be84d 100644
> --- a/v2v/output_vdsm.ml
> +++ b/v2v/output_vdsm.ml
> @@ -81,10 +81,9 @@ object
>      let mp, uuid >        let fields = String.nsplit "/" os
in (* ... "data-center" "UUID" *)
>        let fields = List.rev fields in      (* "UUID"
"data-center" ... *)
> +      let fields = dropwhile ((=) "") fields in
>        match fields with
> -      | "" :: uuid :: rest                 (* handles trailing
"/" case *)
> -      | uuid :: rest
> -          when String.length uuid = 36 ->
> +      | uuid :: rest when String.length uuid = 36 ->
>          let mp = String.concat "/" (List.rev rest) in
>          mp, uuid
>        | _ ->
LGTM.

Thanks,
-- 
Pino Toscano