Re: [gold][patch] Factor the should include member to a new function

[Ian Lance Taylor <iant at google dot com>]

Rafael Espindola <espindola@google.com> writes:

> +namespace should_include
> +{
> +enum should_include
> +  {
> +    NO,
> +    YES,
> +    UNKNOWN
> +  };
> +}

Nested namespaces is not a pattern that gold uses.  Instead, write
something like

enum Should_include
{
  SHOULD_INCLUDE_NO,
  SHOULD_INCLUDE_YES,
  SHOULD_INCLUDE_UNKNOWN
};

There should be a comment before the enum.


> +static should_include::should_include
> +should_include_member(Symbol_table* symtab, const char* sym_name, Symbol** symp,
> +                      std::string& why)

gold follows a policy of never using non-const references as out
parameters, because that is unclear at the call site.  Please change
"why" to be std::string*.


> +{
> +  // In an object file, and therefore in an archive map, an
> +  // '@' in the name separates the symbol name from the
> +  // version name.  If there are two '@' characters, this is
> +  // the default version.
> +  const char* ver = strchr(sym_name, '@');
> +  bool def = false;
> +  if (ver != NULL)
> +    {
> +      size_t symlen = ver - sym_name;
> +      char tmpbuf[symlen + 1];

This is a dynamic array, which is a g++ extension not permitted in
standard C++.  Also, symbol names can be arbitrarily long, and should
not be stored on the stack.  You need to use a memory buffer here.
For efficiency, you will probably need to pass in the address of a
memory buffer and the address of its size, and update those as the
buffer needs to grow.


> +  if (sym == NULL)
> +    {
> +      // Check whether the symbol was named in a -u option.
> +      if (!parameters->options().is_undefined(sym_name))
> +        {
> +          return should_include::UNKNOWN;
> +        }

You don't really need braces around a single statement, but you can
use them if you like.  There is another case below.


> +              /* fall through */

Even trivial comments are complete sentences:
  /* Fall through.  */

> +            case should_include::UNKNOWN:
> +              continue;

Normally a switch statement is more clear, but I'm not entirely
convinced that a switch statement which buries a continue is clearer.
I think a couple of if statements might be more appropriate here.


Please adjust and resend.

Ian