Code organisation

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Code organisation

mmuetzel
On Windows, the functions "getenv" and "putenv" still fail with Octave dev if
one wants to query or set environment variables containing non-ASCII characters.
This implies that some functions (e.g. "which") are failing if a user with
non-ASCII characters in their name tries to run Octave.

I have a patch that switches to using the Unicode API for these functions.
However, I'm unsure how that should integrate into the code base.
For "putenv" that's pretty clear because we are already using a wrapper for
gnulib.
"getenv" is in the namespace "octave::sys::env".
Should I add the OS dependent code directly in "octave::sys::env:do_getenv"? Or
is it cleaner to add a new wrapper "octave::sys::getenv_wrapper" in lo-sysdep
(see attached patch)?

Thanks for any advise
Markus

getenv_unicode.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Code organisation

Mike Miller-4
On Sun, Sep 23, 2018 at 18:48:33 +0200, "Markus M├╝tzel" wrote:
> I have a patch that switches to using the Unicode API for these functions.
> However, I'm unsure how that should integrate into the code base.

I am also unsure, but I will share some thoughts.

> For "putenv" that's pretty clear because we are already using a wrapper for
> gnulib.

Not entirely clear, because we do use a gnulib wrapper, which is then
called by octave_putenv, which is finally called by
octave::sys::env::putenv. Changes to behavior could go in either
octave_putenv or in octave_putenv_wrapper.

Also keep in mind that the gnulib putenv advertises that it allows
removing environment variables. We currently have an open bug about
unsetenv not working on Windows, that may be solved by using the gnulib
putenv function. With your patch that would become more complicated.

> "getenv" is in the namespace "octave::sys::env".
> Should I add the OS dependent code directly in "octave::sys::env:do_getenv"? Or
> is it cleaner to add a new wrapper "octave::sys::getenv_wrapper" in lo-sysdep
> (see attached patch)?

IMHO, the octave::sys::env member functions should be very simple calls
to lower level functions. The class really only exists for caching
certain system values that don't change often.

Also IMHO, the wrapper functions in liboctave/wrappers should be as
simple as possible, they exist mainly to isolate our C++ libraries from
gnulib's C declarations and header files.

So I think the UTF-8 conversion for both getenv and putenv should be
done in intermediate wrapppers somewhere in liboctave, either in
lo-utils.cc where octave_putenv is now, or in lo-sysdep.cc where you
suggested. But please move octave_putenv to lo-sysdep if that makes more
sense. And please keep in mind the unsolved unsetenv problem that may
need yet another wrapper.

--
mike

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Code organisation

mmuetzel
Mike, thanks for your feedback.
I posted a patch (hopefully) following your suggestions on the bug you
mentioned:
https://savannah.gnu.org/bugs/index.php?53922

Markus



--
Sent from: http://octave.1599824.n4.nabble.com/Octave-Maintainers-f1638794.html