Re: Code organisation

[Mike Miller]

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
Description: PGP signature