Re: [PATCH] Add STATIC_GETENV macro.

["Carlos O'Donell" <carlos at redhat dot com>]

On 11/10/2013 03:55 AM, OndÅej BÃlka wrote:
> On Sun, Nov 10, 2013 at 02:52:21AM -0500, Carlos O'Donell wrote:
>> On 11/09/2013 05:38 AM, OndÅej BÃlka wrote:
>>> Hi,
>>>
>>> This adds a STATIC_GETENV macro for avoiding possible getenv bottlenecks as 
>>> documented in 'Make getenv O(1)' thread.
>>>
>>> As this is useful addition this could be a gnu extension.
>>>
>>> An patch that converts internal getenv uses to STATIC_GETENV will
>>> follow.
>>>
>>> There is question if we should make STATIC_GETENV call a secure_getenv
>>> by default or add STATIC_SECURE_GETENV.
>>>
>>> This depends on if getenv calls in next patch could be replaced by
>>> secure getenv or not.
>>>
>>> So far both choices are possible.
>>>
>>> 	* stdlib/stdlib.h (STATIC_GETENV): Add.
>>> 	* manual/startup.texi (Environment Access): Document STATIC_GETENV.
>>> 	* NEWS: Mention STATIC_GETENV.
>>
>> I like where this is going, but new functions like this will need
>> quite a bit of discussion, and it isn't clear to me what uses users
>> would use this where they wouldn't just cache the value themselves?
>>
> How many will? When writing code getenv/STATIC_GETENV choice is mostly
> automatic, doing caching less so.
> 
> It involves adding static variable and choosing appropriate name, if
> conditional and if you take standard as seriously as Rich using atomic
> operations to set result.
> 
> Also when one want to refactoring with codebase containing twenty
> getenvs then with STATIC_GETENV its running a script and checking which
> parts makes sense.
> As user it is first checking which parts make sense and then rewrite
> needed parts which is a repetitive task so when one looks how much he
> should do he gives up.

Let me think about this for a while.

I'm not happy with the name STATIC_GETENV, I wish we could express
the behaviour rather than implementation as part of the name, but
that's just my opinion e.g. GETENV_CACHED or something similar.

The other issue is that we must support users compiling programs
with much older compilers than we support building glibc. When
were the __sync* builtins added? Can you always rely on them?
I thought they were being deprecated in favour of the atomic
builtins? I think you need several ifdefs there to account for
various ages of compilers.
  
>> One of the goals here is that we don't turn this into a "everything
>> and the kitchen sink" library.
>>
> There is no risk of that, it already happened.

That made me laugh :-)

Cheers,
Carlos.