Re: [PATCH v4] C++ify gdb/common/environ.c

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, June 16 2017, Pedro Alves wrote:

>>> Nit: I find it a bit odd that the ctors/dtors are short but 
>>> defined out of line, while this function is defined inline.
>>> If I was looking at controlling what the compiler could inline,
>>> then I'd do it the other way around -- small ctor/dtor in
>>> the header, and this larger function out of line in the .c file.
>> 
>> Question: if I define a method inside the class, does this implicitly
>> tell the compiler that I want to inline it, as oppose to defining the
>> method outside?
>
> It's not about inside vs outside.  It's about the compiler seeing the
> body when compiling a foo.c file that includes the gdb_environ.h header.
> The compiler is invoked on a per-compilation-unit base.  If you put the
> method's definition outside the class but still in the gdb_environ.h header,
> then the compiler would still be able to inline the method's body in the
> foo.c compilation unit if it so chooses.  Of course, then you'd run into
> multiple definition problems at link time.  So you can then mark
> the definition as inline explicitly.  But that's no different from putting a
> free function's definition in a header.
>
> If you put the method instead in gdb_environ.c instead, then when the compiler
> is compiling compilation unit foo.c, it has no idea what the body of
> the method is, so it can't inline it.  Unless you build with -flto,
> of course.

Ah, of course, thanks for the explanation, it makes sense obviously.

>>> So we either always add a NULL to the vector, or we
>>> change gdb_environ::get_char_vector instead, like:
>>>
>>>  char **
>>>  gdb_environ::get_char_vector () const
>>>  {
>>>    if (m_environ_vector.empty ())
>>>      {
>>>        static const char *const empty_envp[1] = { NULL };
>>>        return const_cast<char **> (empty_envp);
>>>      }
>>>    return const_cast<char **> (&m_environ_vector[0]);
>>>  }
>>>
>>> This is OK because execve etc. are garanteed to never change
>>> the envp they're passed.
>> 
>> Oh, good catch.  I prefer to just initialize the vector with a NULL
>> value in the ctor; will do that now.
>
> I'd prefer the other option.  Because then constructing gdb_environ
> is dirt cheap and doesn't require heap memory.  We're constructing one
> environ per inferior, even if we end up not setting any variable
> [now thinking ahead to when we make this work with remote].

I guess I should always implement the option that I *don't* prefer...

Anyway, v5 is ready, should be arriving at your INBOX soon.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/