Janitorial issues: Man-getpage-in-background

Janitorial issues: Man-getpage-in-background

[Kai Großjohann]

The function Man-getpage-in-background uses two different ways to set
environment variables:

(a) (let ((process-environment (copy-sequence process-environment)))
      (setenv "FOO" "bar")
      ...)

(b) (let ((process-environment (cons "FOO=bar" process-environment)))
      ...)

It looks really strange to see them both in the same function...

Okay to unify?  (I'd choose (a) unless there are objections.)
-- 
A preposition is not a good thing to end a sentence with.

Re: Janitorial issues: Man-getpage-in-background

[Richard Stallman]

    (a) (let ((process-environment (copy-sequence process-environment)))
	  (setenv "FOO" "bar")
	  ...)

    (b) (let ((process-environment (cons "FOO=bar" process-environment)))
	  ...)

    It looks really strange to see them both in the same function...

    Okay to unify?  (I'd choose (a) unless there are objections.)

Ok.

Re: Janitorial issues: Man-getpage-in-background

[Kevin Rodgers]

Kai Großjohann wrote:

> The function Man-getpage-in-background uses two different ways to set
> environment variables:
> 
> (a) (let ((process-environment (copy-sequence process-environment)))
>       (setenv "FOO" "bar")
>       ...)
> 
> (b) (let ((process-environment (cons "FOO=bar" process-environment)))
>       ...)
> 
> It looks really strange to see them both in the same function...
> 
> Okay to unify?  (I'd choose (a) unless there are objections.)

Objection!  (I've always wanted to say that :-)

On the basis of unnecessary cons'ing.

-- 
<a href="mailto:&lt;kevin.rodgers&#64;ihs.com&gt;">Kevin Rodgers</a>

Re: Janitorial issues: Man-getpage-in-background

[Kai Großjohann]

Kevin Rodgers <kevin.rodgers@ihs.com> writes:

> Kai Großjohann wrote:
>
>> The function Man-getpage-in-background uses two different ways to set
>> environment variables:
>> (a) (let ((process-environment (copy-sequence process-environment)))
>>       (setenv "FOO" "bar")
>>       ...)
>> (b) (let ((process-environment (cons "FOO=bar" process-environment)))
>>       ...)
>> It looks really strange to see them both in the same function...
>> Okay to unify?  (I'd choose (a) unless there are objections.)
>
> Objection!  (I've always wanted to say that :-)
>
> On the basis of unnecessary cons'ing.

It seems you do not object to the unification, just to my choice of
(a)?

It seems that setenv does quite a bit of work in addition to consing
stuff onto process-environment.  I'm not sure it is a good idea to
skip using setenv.  Wasn't there a discussion about having setenv do
something additional to frobbing process-environment?  It was some
moons ago.  I'm not talking about the multibyte issue that was
recently mentioned.  Hmmm...
-- 
A preposition is not a good thing to end a sentence with.

Re: Janitorial issues: Man-getpage-in-background

[Richard Stallman]

    > On the basis of unnecessary cons'ing.

I doubt the amount of consing is significant enough
*in this context* to be an important factor in the decision.

Re: Janitorial issues: Man-getpage-in-background

[Kai Großjohann]

Richard Stallman <rms@gnu.org> writes:

>     > On the basis of unnecessary cons'ing.
>
> I doubt the amount of consing is significant enough
> *in this context* to be an important factor in the decision.

OK.  I've now used (setenv "GROFF_NO_SGR" "1") instead of let-binding
process-environment.

This brings up a new question: GROFF_NO_SGR is only set when
call-process is used to start man, it is not set when start-process is
used.

Is this intentional?
-- 
A preposition is not a good thing to end a sentence with.

Re: Janitorial issues: Man-getpage-in-background

[Stefan Monnier]

> It seems that setenv does quite a bit of work in addition to consing
> stuff onto process-environment.  I'm not sure it is a good idea to
> skip using setenv.  Wasn't there a discussion about having setenv do
> something additional to frobbing process-environment?  It was some
> moons ago.  I'm not talking about the multibyte issue that was
> recently mentioned.  Hmmm...

You're probably referring to the problem of C libraries that
use envvars and whose functions are called by Emacs.


	Stefan

Re: Janitorial issues: Man-getpage-in-background

[Kai Großjohann]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> writes:

>> It seems that setenv does quite a bit of work in addition to consing
>> stuff onto process-environment.  I'm not sure it is a good idea to
>> skip using setenv.  Wasn't there a discussion about having setenv do
>> something additional to frobbing process-environment?  It was some
>> moons ago.  I'm not talking about the multibyte issue that was
>> recently mentioned.  Hmmm...
>
> You're probably referring to the problem of C libraries that
> use envvars and whose functions are called by Emacs.

Yes, maybe the discussion was about whether the `setenv' Lisp function
should call the `setenv(2)' C function.

If Man-getpage-in-background ever needs to set $TZ, we would be in
trouble...
-- 
A preposition is not a good thing to end a sentence with.

Re: Janitorial issues: Man-getpage-in-background

[Stefan Monnier]

> The function Man-getpage-in-background uses two different ways to set
> environment variables:
> 
> (a) (let ((process-environment (copy-sequence process-environment)))
>       (setenv "FOO" "bar")
>       ...)
> 
> (b) (let ((process-environment (cons "FOO=bar" process-environment)))
>       ...)
> 
> It looks really strange to see them both in the same function...
> 
> Okay to unify?  (I'd choose (a) unless there are objections.)

I'd choose (b) to avoid unnecessary consing (via copying).


	Stefan