[PATCH 43/49] analyzer: new file: exploded-graph.h

[David Malcolm]

On Wed, 2019-12-11 at 13:04 -0700, Jeff Law wrote:
> On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> > This patch adds exploded_graph and related classes, for managing
> > exploring paths through the user's code as a directed graph
> > of <point, state> pairs.
> > 
> > gcc/ChangeLog:
> > 	* analyzer/exploded-graph.h: New file.
> > ---
> >  gcc/analyzer/exploded-graph.h | 754
> > ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 754 insertions(+)
> >  create mode 100644 gcc/analyzer/exploded-graph.h
> > 
> > diff --git a/gcc/analyzer/exploded-graph.h b/gcc/analyzer/exploded-
> > graph.h
> > new file mode 100644
> > index 0000000..f97d2b6
> > --- /dev/null
> > +++ b/gcc/analyzer/exploded-graph.h
> > @@ -0,0 +1,754 @@
> > +/* Classes for managing a directed graph of <point, state> pairs.
> > +   Copyright (C) 2019 Free Software Foundation, Inc.
> > +   Contributed by David Malcolm <dmalcolm@redhat.com>.
> > +
> > +This file is part of GCC.
> > +
> > +GCC is free software; you can redistribute it and/or modify it
> > +under the terms of the GNU General Public License as published by
> > +the Free Software Foundation; either version 3, or (at your
> > option)
> > +any later version.
> > +
> > +GCC is distributed in the hope that it will be useful, but
> > +WITHOUT ANY WARRANTY; without even the implied warranty of
> > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +General Public License for more details.
> > +
> > +You should have received a copy of the GNU General Public License
> > +along with GCC; see the file COPYING3.  If not see
> > +<http://www.gnu.org/licenses/>;;.  */
> > +
> > +#ifndef GCC_ANALYZER_EXPLODED_GRAPH_H
> > +#define GCC_ANALYZER_EXPLODED_GRAPH_H
> > +
> > +#include "fibonacci_heap.h"
> > +#include "analyzer/analyzer-logging.h"
> > +#include "analyzer/constraint-manager.h"
> > +#include "analyzer/diagnostic-manager.h"
> > +#include "analyzer/program-point.h"
> > +#include "analyzer/program-state.h"
> > +#include "analyzer/shortest-paths.h"
> > +
> > +//////////////////////////////////////////////////////////////////
> > //
> NIT.  Is there some reason you don't just use whitespace for these
> kind
> of vertical separators.  THe more I see them, the more they bug me,
> probably because that's not been the style we've used for GCC.

I've been using them to highlight new "chapters" in a source file;
typically there's another comment following them.  I'm happy to drop
the one where there isn't a trailing comment, but, I'm hoping there's
an acceptable way to have an "section-header"-style comment.

We have "^L" in a bunch of places, but my editor (emacs) doesn't do a
great job of highlighting them.

There's some precedence for: e.g. 

gengtype.c:5094:/******* Manage input files.  ******/

lto-section-in.c has e.g.:

/*****************************************************************************/
/* Record renamings of static declarations                                   */
/*****************************************************************************/

tree-vect-loop-manip.c has:

/*************************************************************************
  Simple Loop Peeling Utilities

  Utilities to support loop peeling for vectorization purposes.
 *************************************************************************/

but I suspect given how sporadic these are that this stuff snuck past
review.

Alternatively I can just drop these and retain the trailing comments,
keeping their formatting.

> > ///////
> > +
> > +/* Concrete implementation of region_model_context, wiring it up
> > to
> > the
> > +   rest of the analysis engine.  */
> > +
> > +class impl_region_model_context : public region_model_context,
> > public log_user
> Multiple inheritance?  Is it really needed?  Can we handle via
> composition instead?

It's handy here, but I can probably eliminate it.

> 
> > +/* A <program_point, program_state> pair, used internally by
> > +   exploded_node as its immutable data, and as a key for
> > identifying
> > +   exploded_nodes we've already seen in the graph.  */
> > +
> > +struct point_and_state
> Shouldn't this be a class?

Will fix (originally was just a pair of public fields, but then I added
the cached hash value, so there's an argument for "class-ifying" this).

> +
> > +/* Per-program_point data for an exploded_graph.  */
> > +
> > +struct per_program_point_data
> Here too?  THere may be others.  I'd suggest reviewing all your
> "structs" and determine if we're better off calling them
> "class".  I'm
> not going to insist on it though since I think the last discussion in
> this space was to relax the conventions :-)

Both fields are public, but they're not-POD.

I'll look into them.

> > +
> > +class exploded_graph : public digraph<eg_traits>, public log_user
> Multiple inheritance again?

Again, it's handy here, but I think I can probably eliminate it.



Dave