pending instrument-control release

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

pending instrument-control release

Olaf Till-2
John and Stefan,

in the proposed instrument-control-0.3.0 NEWS advertises and UDP
overloaded function "close", which is not in INDEX. Where is that
function defined?

A minor issue is that the indentation in NEWS is wrong for this item.

Note that releases of community packages should only be tagged after
the release is ready (typically I'm doing this).


Next issues probably are not release-critical, but maybe one of you
wants to have them dealt with before the release. If so, please tell
me.


As you probably know, symbols defined in an oct-file are not "seen" by
other oct-files (and by Octave), due to the way oct-files are loaded
by Octave. This is reasonable, but must be considered. You have dealt
with this by defining the global variable type_loaded (which is
supposed to indicate if ...::register_type() has already been called)
in several oct-files. This causes ...::register_type() of a new type
to be called once for each loaded oct-file which uses this type. This
will work, as long as Octave tolerates calling register_type() several
times for the same new type. But it's certainly not very clean. And if
this part of instrument-control should ever be incorporated into core
Octave, there may be problems due to multiple declarartions of the
same global variable.

The solution is to link all files containing DEFUN_DLD for a certain
new type together into single type-specific oct-files, and to
introduce PKG_ADD commands containing autoload(...). And, the global
variable type_loaded should be replaced by static variables within
member functions of the respective types.

There may be similar issues elsewhere.


In the future, the coding style should be adapted to that of Octave
everywhere. It deviates from it at some places.

The latter point also means that explicite 'this->...' should only be
used if necessary.


If compiled with -Wall, there are several warnings of 'defined but not
used', in particular in the context of operator definition, so the
latter is probably not successfully implemented.


With development versions of Octave, there are many deprecation
warnings. They don't need to be dealt with now, but doing so could
make the release longer usable... Since I have a standard frame for
this, do you want me to apply it? If yes, before or after the release?

Olaf

--
public key id EAFE0591, e.g. on x-hkp://pool.sks-keyservers.net

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: pending instrument-control release

JohnD
My comments below

> -----Original Message-----
> From: Olaf Till [mailto:[hidden email]]
> Sent: Wednesday, August 09, 2017 3:13 PM
> To: Octave Maintainers
> Cc: JohnD; Stefan Mahr
> Subject: pending instrument-control release
>
> John and Stefan,
>
> in the proposed instrument-control-0.3.0 NEWS advertises and UDP
overloaded
> function "close", which is not in INDEX. Where is that function defined?


You are right - no close - I will remove it.

>
> A minor issue is that the indentation in NEWS is wrong for this item.
>

I don't see anything different on it compared to the other entries - I do
see that resolvehost is off by a space so will fix that. Let me know if I am
missing something obvious.

> Note that releases of community packages should only be tagged after the
> release is ready (typically I'm doing this).
>

The instructions in the makefile suggest tagging it, so will remove that
line as well.

>
> Next issues probably are not release-critical, but maybe one of you wants
to
> have them dealt with before the release. If so, please tell me.
>
>
> As you probably know, symbols defined in an oct-file are not "seen" by
other
> oct-files (and by Octave), due to the way oct-files are loaded by Octave.
This is
> reasonable, but must be considered. You have dealt with this by defining
the
> global variable type_loaded (which is supposed to indicate if
...::register_type()
> has already been called) in several oct-files. This causes
...::register_type() of a
> new type to be called once for each loaded oct-file which uses this type.
This
> will work, as long as Octave tolerates calling register_type() several
times for
> the same new type. But it's certainly not very clean. And if this part of
> instrument-control should ever be incorporated into core Octave, there may
be
> problems due to multiple declarartions of the same global variable.
>
> The solution is to link all files containing DEFUN_DLD for a certain new
type
> together into single type-specific oct-files, and to introduce PKG_ADD
> commands containing autoload(...). And, the global variable type_loaded
should
> be replaced by static variables within member functions of the respective
> types.
>
> There may be similar issues elsewhere.
>

I see in the database package that you register the type in the new octave
value type - is that an acceptable solution we can use?

>
> In the future, the coding style should be adapted to that of Octave
everywhere.
> It deviates from it at some places.
>

I don't believe the coding style matches octave anywhere except by accident
:) Will try getting in order for a futures release, not now.

> The latter point also means that explicite 'this->...' should only be used
if
> necessary.
>
>
> If compiled with -Wall, there are several warnings of 'defined but not
used', in
> particular in the context of operator definition, so the latter is
probably not
> successfully implemented.
>

I havent done anything with the operators before , so don't know - are they
there and will be used by calls from octave itself rather than from in the
oct file code?

Ie:

A = serial()
B = A

If A == B
  display " the same object"
endif

>
> With development versions of Octave, there are many deprecation warnings.
> They don't need to be dealt with now, but doing so could make the release
> longer usable... Since I have a standard frame for this, do you want me to
apply
> it? If yes, before or after the release?
>

I saw the warnings in dev octave, but, like you was reluctant to do any
changes before the official octave release unless it was stopping it from
building.

If it is something easy for you, and you have the time I am ok with you
making the change now, if you have other stuff to do, then after the release
is also fine - let me know.

> Olaf
>
> --
> public key id EAFE0591, e.g. on x-hkp://pool.sks-keyservers.net



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pending instrument-control release

Olaf Till-2
On Wed, Aug 09, 2017 at 03:50:06PM -0400, JohnD wrote:

> My comments below
>
> > -----Original Message-----
> > From: Olaf Till [mailto:[hidden email]]
> > Sent: Wednesday, August 09, 2017 3:13 PM
> > To: Octave Maintainers
> > Cc: JohnD; Stefan Mahr
> > Subject: pending instrument-control release
> >
> > <snip>
> >
> > A minor issue is that the indentation in NEWS is wrong for this item.
> >
>
> I don't see anything different on it compared to the other entries - I do
> see that resolvehost is off by a space so will fix that. Let me know if I am
> missing something obvious.
I've pushed a minor correction.

> > <snip>
> >
> > Next issues probably are not release-critical, but maybe one of
> you wants to > have them dealt with before the release. If so,
> please tell me.
> >
> >
> > As you probably know, symbols defined in an oct-file are not
> > "seen" by other oct-files (and by Octave), due to the way
> > oct-files are loaded by Octave.  This is reasonable, but must be
> > considered. You have dealt with this by defining the global
> > variable type_loaded (which is supposed to indicate if
> > ...::register_type() has already been called) in several
> > oct-files. This causes ...::register_type() of a new type to be
> > called once for each loaded oct-file which uses this type.  This
> > will work, as long as Octave tolerates calling register_type()
> > several times for the same new type. But it's certainly not very
> > clean. And if this part of instrument-control should ever be
> > incorporated into core Octave, there may be problems due to
> > multiple declarartions of the same global variable.
> >
> > The solution is to link all files containing DEFUN_DLD for a certain new
> > type
> > together into single type-specific oct-files, and to introduce PKG_ADD
> > commands containing autoload(...). And, the global variable type_loaded
> > should
> > be replaced by static variables within member functions of the respective
> > types.
> >
> > There may be similar issues elsewhere.
>
> I see in the database package that you register the type in the new octave
> value type - is that an acceptable solution we can use?
This should be better. But still, as long as separate oct-files are
used, the static flag variables in the methods are not seen across
oct-files and ...::register_type() will be called more than once for a
type.

> > <snip>
> >
> > If compiled with -Wall, there are several warnings of 'defined but
> > not used', in particular in the context of operator definition, so
> > the latter is probably not successfully implemented.
>
> I havent done anything with the operators before , so don't know - are they
> there and will be used by calls from octave itself rather than from in the
> oct file code?

I'm not prepared to dig into Octaves concept of operator definition
now... But however it's done, if the compiler complains about 'defined
but not used', I think this part of code can't be useful in any way.

Let's put this off until later.

> <snip>
>
> > With development versions of Octave, there are many deprecation
> > warnings.  They don't need to be dealt with now, but doing so
> > could make the release longer usable... Since I have a standard
> > frame for this, do you want me to apply it? If yes, before or
> > after the release?
>
> I saw the warnings in dev octave, but, like you was reluctant to do any
> changes before the official octave release unless it was stopping it from
> building.
>
> If it is something easy for you, and you have the time I am ok with
> you making the change now, if you have other stuff to do, then after
> the release is also fine - let me know.
So I'll do it after the release.

I understand that we can put off all unresolved items until later, so
I'll push the release. You don't need to regenerate the tarballs,
since the root Makefile works fine.

Olaf

--
public key id EAFE0591, e.g. on x-hkp://pool.sks-keyservers.net

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: pending instrument-control release

JohnD

> -----Original Message-----
> From: Olaf Till [mailto:[hidden email]]
> Sent: Thursday, August 10, 2017 4:31 AM
> To: JohnD
> Cc: 'Octave Maintainers'; 'Stefan Mahr'
> Subject: Re: pending instrument-control release
>


-- snipped --
 
>
> I understand that we can put off all unresolved items until later, so I'll
push the
> release. You don't need to regenerate the tarballs, since the root
Makefile
> works fine.
>
> Olaf
>

Thanks!


Loading...