Re: [PATCH] Refactor handle_section_attribute to reduce nesting and distinguish error cases

[Trevor Saunders <tsaunders at mozilla dot com>]

On Sun, Aug 24, 2014 at 01:58:52PM -0700, Andrew Pinski wrote:
> On Sun, Aug 24, 2014 at 1:42 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > handle_section_attribute contains many levels of nested conditionals and
> > branching code flow paths, with the error cases sometimes in the else
> > case and sometimes in the if case.  Simplify the code flow into a series
> > of potential failure cases ending with the successful path, with no
> > nesting and no else clauses.
> > ---
> >
> > I started to work on an extension to the section attribute, and found
> > myself cleaning up the logic in handle_section_attribute; I wanted to
> > send this cleanup patch separately.
> >
> > I'm not a GCC committer, so I'm looking both for review and for someone
> > to commit this patch.
> >
> >  gcc/ChangeLog           |  5 +++
> >  gcc/c-family/c-common.c | 91 +++++++++++++++++++++++++------------------------
> >  2 files changed, 51 insertions(+), 45 deletions(-)
> >
> > diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> > index 703a489..0286bfb 100644
> > --- a/gcc/ChangeLog
> > +++ b/gcc/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2014-08-24  Josh Triplett  <josh@joshtriplett.org>
> > +
> > +       * c-family/c-common.c (handle_section_attribute): Refactor to reduce
> > +       nesting and distinguish between error cases.
> > +
> >  2014-08-24  Oleg Endo  <olegendo@gcc.gnu.org>
> >
> >         PR target/61996
> > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> > index 58b9763..a63eedf 100644
> > --- a/gcc/c-family/c-common.c
> > +++ b/gcc/c-family/c-common.c
> > @@ -7387,58 +7387,59 @@ handle_section_attribute (tree *node, tree ARG_UNUSED (name), tree args,
> >  {
> >    tree decl = *node;
> >
> > -  if (targetm_common.have_named_sections)
> > +  if (!targetm_common.have_named_sections)
> >      {
> > -      user_defined_section_attribute = true;
> > +      error_at (DECL_SOURCE_LOCATION (*node),
> > +               "section attributes are not supported for this target");
> > +      goto fail;
> 
> Why not move this to a different function and then do:
> 
> if (function(...))
>   set_decl_section_name (decl, TREE_STRING_POINTER (TREE_VALUE (args)));
> else
>   *no_add_attrs = true;
> 
> return NULL_TREE;
> 
> This is a way to get away from using gotos.

or you could just set no_add_attrs to true at the start of the function
and only set it to false when you know the attr is valid, which saves
the weirdness of a second function.

Trev

> 
> Thanks,
> Andrew
> 
> 
> > +    }
> >
> > -      if ((TREE_CODE (decl) == FUNCTION_DECL
> > -          || TREE_CODE (decl) == VAR_DECL)
> > -         && TREE_CODE (TREE_VALUE (args)) == STRING_CST)
> > -       {
> > -         if (TREE_CODE (decl) == VAR_DECL
> > -             && current_function_decl != NULL_TREE
> > -             && !TREE_STATIC (decl))
> > -           {
> > -             error_at (DECL_SOURCE_LOCATION (decl),
> > -                       "section attribute cannot be specified for "
> > -                       "local variables");
> > -             *no_add_attrs = true;
> > -           }
> > +  user_defined_section_attribute = true;
> >
> > -         /* The decl may have already been given a section attribute
> > -            from a previous declaration.  Ensure they match.  */
> > -         else if (DECL_SECTION_NAME (decl) != NULL
> > -                  && strcmp (DECL_SECTION_NAME (decl),
> > -                             TREE_STRING_POINTER (TREE_VALUE (args))) != 0)
> > -           {
> > -             error ("section of %q+D conflicts with previous declaration",
> > -                    *node);
> > -             *no_add_attrs = true;
> > -           }
> > -         else if (TREE_CODE (decl) == VAR_DECL
> > -                  && !targetm.have_tls && targetm.emutls.tmpl_section
> > -                  && DECL_THREAD_LOCAL_P (decl))
> > -           {
> > -             error ("section of %q+D cannot be overridden", *node);
> > -             *no_add_attrs = true;
> > -           }
> > -         else
> > -           set_decl_section_name (decl,
> > -                                  TREE_STRING_POINTER (TREE_VALUE (args)));
> > -       }
> > -      else
> > -       {
> > -         error ("section attribute not allowed for %q+D", *node);
> > -         *no_add_attrs = true;
> > -       }
> > +  if (TREE_CODE (decl) != FUNCTION_DECL && TREE_CODE (decl) != VAR_DECL)
> > +    {
> > +      error ("section attribute not allowed for %q+D", *node);
> > +      goto fail;
> >      }
> > -  else
> > +
> > +  if (TREE_CODE (TREE_VALUE (args)) != STRING_CST)
> >      {
> > -      error_at (DECL_SOURCE_LOCATION (*node),
> > -               "section attributes are not supported for this target");
> > -      *no_add_attrs = true;
> > +      error ("section attribute argument not a string constant");
> > +      goto fail;
> > +    }
> > +
> > +  if (TREE_CODE (decl) == VAR_DECL
> > +      && current_function_decl != NULL_TREE
> > +      && !TREE_STATIC (decl))
> > +    {
> > +      error_at (DECL_SOURCE_LOCATION (decl),
> > +                "section attribute cannot be specified for local variables");
> > +      goto fail;
> >      }
> >
> > +  /* The decl may have already been given a section attribute
> > +     from a previous declaration.  Ensure they match.  */
> > +  if (DECL_SECTION_NAME (decl) != NULL
> > +      && strcmp (DECL_SECTION_NAME (decl),
> > +                 TREE_STRING_POINTER (TREE_VALUE (args))) != 0)
> > +    {
> > +      error ("section of %q+D conflicts with previous declaration", *node);
> > +      goto fail;
> > +    }
> > +
> > +  if (TREE_CODE (decl) == VAR_DECL
> > +      && !targetm.have_tls && targetm.emutls.tmpl_section
> > +      && DECL_THREAD_LOCAL_P (decl))
> > +    {
> > +      error ("section of %q+D cannot be overridden", *node);
> > +      goto fail;
> > +    }
> > +
> > +  set_decl_section_name (decl, TREE_STRING_POINTER (TREE_VALUE (args)));
> > +  return NULL_TREE;
> > +
> > +fail:
> > +  *no_add_attrs = true;
> >    return NULL_TREE;
> >  }
> >
> > --
> > 2.1.0
> >