This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
C++ coding conventions: namespaces, references and getters (was Re: [PATCH 2/2] Introduce beginnings of a pipeline class.)
- From: David Malcolm <dmalcolm at redhat dot com>
- To: Martin Jambor <mjambor at suse dot cz>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Mon, 29 Jul 2013 14:20:02 -0400
- Subject: C++ coding conventions: namespaces, references and getters (was Re: [PATCH 2/2] Introduce beginnings of a pipeline class.)
- References: <1374678544-8678-1-git-send-email-dmalcolm at redhat dot com> <1374678544-8678-3-git-send-email-dmalcolm at redhat dot com> <20130725130845 dot GB12455 at virgil dot suse>
On Thu, 2013-07-25 at 15:08 +0200, Martin Jambor wrote:
> Hi,
>
> I don't know why it's me again but again I do have a few comments.
Thanks for looking over the patch.
> One global remark first: If we are going to start using the gcc
> namespace (I understand it you need for isolation of symbols once you
> use gcc as library, right?), I'm wondering whether mixing "using
> namespace gcc" and explicit identifier qualifications is a good idea.
> We have never had a discussion about namespace conventions but I'd
> suggest that you add the using directive at the top of all gcc source
> files that need it and never use explicit gcc:: qualification (unless
> there is a reason for it, of course, like in symbol definitions).
>
> But perhaps someone else with more C++ experience disagrees?
http://gcc.gnu.org/codingconventions.html#Namespace_Use says:
> "Namespaces are encouraged. All separable libraries should have a
unique global namespace."
[...snip...]
> "Header files should have neither using directives nor namespace-scope
using declarations."
and the rationale doc says:
> "Using them within an implementation file can help conciseness."
However, there doesn't seem to be a discussion on the merits of the
various forms of "using" directives.
These aren't the GCC coding conventions, but the Google conventions:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces
forbid using directives of the form:
using NAMESPACE;
but suggest using directives of the form:
using NAMESPACE::NAME;
to pick out individual names from a namespace, though *not* in global
scope in a header (to avoid polluting the global namespace).
I like this approach - how about using it for frequently used names in
a .c/.cc file, keeping the names alphabetizing by "fully qualified
path", so e.g.:
#include "foo.h"
...
#include "bar.h"
using gcc::context;
using gcc::pass_manager;
...etc, individually grabbing the names we'll be needing
// code follows
and thus avoiding grabbing whole namespaces, whilst keeping the code
concise.
I may want to violate that rule within gtype-desc.c, as per:
http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01262.html
to simplify handling of GTY and "gcc::"
> A few more comments inline:
Likewise
[...]
> > diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> > index b82c2e0..dc489fb 100644
> > --- a/gcc/cgraphunit.c
> > +++ b/gcc/cgraphunit.c
> > @@ -194,6 +194,8 @@ along with GCC; see the file COPYING3. If not see
> > #include "except.h"
> > #include "cfgloop.h"
> > #include "regset.h" /* FIXME: For reg_obstack. */
> > +#include "context.h"
> > +#include "pipeline.h"
> >
> > /* Queue of cgraph nodes scheduled to be added into cgraph. This is a
> > secondary queue used during optimization to accommodate passes that
> > @@ -478,6 +480,7 @@ cgraph_finalize_function (tree decl, bool nested)
> > void
> > cgraph_add_new_function (tree fndecl, bool lowered)
> > {
> > + gcc::pipeline &passes = g->get_passes ();
> > struct cgraph_node *node;
> > switch (cgraph_state)
> > {
> > @@ -508,7 +511,7 @@ cgraph_add_new_function (tree fndecl, bool lowered)
> > push_cfun (DECL_STRUCT_FUNCTION (fndecl));
> > gimple_register_cfg_hooks ();
> > bitmap_obstack_initialize (NULL);
> > - execute_pass_list (all_lowering_passes);
> > + execute_pass_list (passes.all_lowering_passes);
> > execute_pass_list (pass_early_local_passes.pass.sub);
> > bitmap_obstack_release (NULL);
> > pop_cfun ();
> > @@ -640,7 +643,7 @@ analyze_function (struct cgraph_node *node)
> >
> > gimple_register_cfg_hooks ();
> > bitmap_obstack_initialize (NULL);
> > - execute_pass_list (all_lowering_passes);
> > + execute_pass_list (g->get_passes ().all_lowering_passes);
> > free_dominance_info (CDI_POST_DOMINATORS);
> > free_dominance_info (CDI_DOMINATORS);
> > compact_blocks ();
> > @@ -1588,7 +1591,7 @@ expand_function (struct cgraph_node *node)
> > /* Signal the start of passes. */
> > invoke_plugin_callbacks (PLUGIN_ALL_PASSES_START, NULL);
> >
> > - execute_pass_list (all_passes);
> > + execute_pass_list (g->get_passes ().all_passes);
> >
> > /* Signal the end of passes. */
> > invoke_plugin_callbacks (PLUGIN_ALL_PASSES_END, NULL);
> > @@ -1807,6 +1810,8 @@ output_in_order (void)
> > static void
> > ipa_passes (void)
> > {
> > + gcc::pipeline &passes = g->get_passes ();
> > +
> > set_cfun (NULL);
> > current_function_decl = NULL;
> > gimple_register_cfg_hooks ();
> > @@ -1816,7 +1821,7 @@ ipa_passes (void)
> >
> > if (!in_lto_p)
> > {
> > - execute_ipa_pass_list (all_small_ipa_passes);
> > + execute_ipa_pass_list (passes.all_small_ipa_passes);
> > if (seen_error ())
> > return;
> > }
> > @@ -1843,14 +1848,15 @@ ipa_passes (void)
> > cgraph_process_new_functions ();
> >
> > execute_ipa_summary_passes
> > - ((struct ipa_opt_pass_d *) all_regular_ipa_passes);
> > + ((struct ipa_opt_pass_d *) passes.all_regular_ipa_passes);
> > }
> >
> > /* Some targets need to handle LTO assembler output specially. */
> > if (flag_generate_lto)
> > targetm.asm_out.lto_start ();
> >
> > - execute_ipa_summary_passes ((struct ipa_opt_pass_d *) all_lto_gen_passes);
> > + execute_ipa_summary_passes ((struct ipa_opt_pass_d *)
> > + passes.all_lto_gen_passes);
>
> Hehe. I understand you have a lot of work with this, but perhaps you
> could also turn struct opt_pass and its "descendants" into a struct
> hierarchy too? (I'd really use structs, not classes, and for now put
> off any other re-engineering like visibilities and such). And put
> them into pipeline.h? But perhaps not, I'll try to remember to do it
> later :-)
I do this in http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01252.html
> > if (!in_lto_p)
> > ipa_write_summaries ();
> > @@ -1859,7 +1865,7 @@ ipa_passes (void)
> > targetm.asm_out.lto_end ();
> >
> > if (!flag_ltrans && (in_lto_p || !flag_lto || flag_fat_lto_objects))
> > - execute_ipa_pass_list (all_regular_ipa_passes);
> > + execute_ipa_pass_list (passes.all_regular_ipa_passes);
> > invoke_plugin_callbacks (PLUGIN_ALL_IPA_PASSES_END, NULL);
> >
> > bitmap_obstack_release (NULL);
> > @@ -1985,7 +1991,7 @@ compile (void)
> >
> > cgraph_materialize_all_clones ();
> > bitmap_obstack_initialize (NULL);
> > - execute_ipa_pass_list (all_late_ipa_passes);
> > + execute_ipa_pass_list (g->get_passes ().all_late_ipa_passes);
> > symtab_remove_unreachable_nodes (true, dump_file);
> > #ifdef ENABLE_CHECKING
> > verify_symtab ();
> > diff --git a/gcc/context.c b/gcc/context.c
> > index 76e0dde..8ec2e60 100644
> > --- a/gcc/context.c
> > +++ b/gcc/context.c
> > @@ -22,6 +22,12 @@ along with GCC; see the file COPYING3. If not see
> > #include "coretypes.h"
> > #include "ggc.h"
> > #include "context.h"
> > +#include "pipeline.h"
> >
> > /* The singleton holder of global state: */
> > gcc::context *g;
> > +
> > +gcc::context::context()
> > +{
> > + passes_ = new gcc::pipeline(this);
> > +}
>
> Missing spaces before parentheses (yeah, I dislike them too, but...)
I somehow got it into my head that constructors were an exception to
this rule, based on looking at the base-class constructor examples here:
http://gcc.gnu.org/codingconventions.html#Member_Form
which lack a space before parens.
But yes, I'll ensure that "new CLASSNAME ()" invocations have the space.
> > diff --git a/gcc/context.h b/gcc/context.h
> > index 3caf02f..a83f7b2 100644
> > --- a/gcc/context.h
> > +++ b/gcc/context.h
> > @@ -22,14 +22,23 @@ along with GCC; see the file COPYING3. If not see
> >
> > namespace gcc {
> >
> > +class pipeline;
> > +
> > /* GCC's internal state can be divided into zero or more
> > "parallel universe" of state; an instance of this class is one such
> > context of state. */
> > class context
> > {
> > public:
> > + context();
> > +
> > + /* Pass-management. */
> > +
> > + pipeline &get_passes () { gcc_assert (passes_); return *passes_; }
>
> I don't like that you return a reference instead of a pointer. I
> believe that in a project like gcc where we are about to mix C++ code
> with large portion of original C stuff, we should always strongly
> prefer good old pointers to references to avoid confusion. Especially
> in return value types. (Yeah, I know that in some cases there are
> substantial reasons to return references but this does not seem to be
> one of them.)
Note that as per
http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01365.html
we'll use "pass_manager" rather than "pipeline", so this would look
like:
pass_manager &get_passes () { gcc_assert (passes_); return *passes_; }
We were chatting about C++ references on IRC on Friday, and IIRC there
was a strong objection to passing *arguments* that are non-const
references, rather than return values - mutation of *arguments* being
surprising and a place for bugs to arise (though that may have been
Diego who was arguing that, citing
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Arguments
).
I prefer to have get_passes return a reference rather a pointer since
which thing is being referred to isn't going to change, and is non-NULL.
One could write it as a "gcc::pipeline const * passes", but that doesn't
capture the non-NULL-ness of it.
[within the class data, "passes_" needs to be a *pointer* so that the
PCH deserialization can work, in case the object has been relocated].
Having the get_passes method do the assertion of non-NULLness and
dereference means that there's a single place where the non-NULLness is
asserted.
I guess this is a bigger point though: how do GCC maintainers feel about
C++ references in general?
Looking at the GCC Coding Conventions:
http://gcc.gnu.org/codingconventions.html
and
http://gcc.gnu.org/codingrationale.html
I don't see any mention of C++ references.
Are C++ references permissible inside GCC's own code (outside of
libstdc++, of course) and is the above usage sane?
There is another question here, which is how people feel about
accessors/getters?
I deliberately used one here since at my Cauldron talk someone (Roland?)
pointed out that if we want to optimize the single-state build, we can
change the insides of this method in one place and e.g. put the pass
manager in a fixed location in the bss segment, at which point field
accesses are of fixed locations, which is far cleaner that the various
macro based approaches I had proposed. (how would this interact with
references vs pointers, if at all?)
> > - /* Currently empty. */
> > +private:
> > + /* Pass-management. */
> > + pipeline *passes_;
> >
> > }; // class context
> >
> > diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
> > index 4a287f6..d322d9a 100644
> > --- a/gcc/lto-cgraph.c
> > +++ b/gcc/lto-cgraph.c
> > @@ -47,6 +47,8 @@ along with GCC; see the file COPYING3. If not see
> > #include "gcov-io.h"
> > #include "tree-pass.h"
> > #include "profile.h"
> > +#include "context.h"
> > +#include "pipeline.h"
> >
> > static void output_cgraph_opt_summary (void);
> > static void input_cgraph_opt_summary (vec<symtab_node> nodes);
> > @@ -936,6 +938,7 @@ input_node (struct lto_file_decl_data *file_data,
> > enum LTO_symtab_tags tag,
> > vec<symtab_node> nodes)
> > {
> > + gcc::pipeline &passes = g->get_passes ();
>
> The same here and at a few other places. It may be just me not being
> used to references... nevertheless, if someone really wants to use
> them like this, at least make them const and you will save a night of
> frantic debugging to someone, probably to yourself. But I strongly
> prefer pointers... it's hard to describe how strongly I prefer them.
OK. How do others feel? As I said above, I like the above idiom,
since it puts the assertion of non-NULLness in a single place.
FWIW, I've changed the above to use a "using gcc::pass_manager", so in
my working copy it currently reads:
pass_manager &passes = g->get_passes ();
[...snip...]
> > diff --git a/gcc/pipeline.h b/gcc/pipeline.h
> > new file mode 100644
> > index 0000000..37c90d7
> > --- /dev/null
> > +++ b/gcc/pipeline.h
> > @@ -0,0 +1,89 @@
> > +/* pipeline.h - The pipeline of optimization passes
> > + Copyright (C) 2013 Free Software Foundation, Inc.
> > +
> > +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_PIPELINE_H
> > +#define GCC_PIPELINE_H
> > +
> > +class opt_pass;
> > +struct register_pass_info;
> > +
> > +/* Define a list of pass lists so that both passes.c and plugins can easily
> > + find all the pass lists. */
> > +#define GCC_PASS_LISTS \
> > + DEF_PASS_LIST (all_lowering_passes) \
> > + DEF_PASS_LIST (all_small_ipa_passes) \
> > + DEF_PASS_LIST (all_regular_ipa_passes) \
> > + DEF_PASS_LIST (all_lto_gen_passes) \
> > + DEF_PASS_LIST (all_passes)
> > +
> > +#define DEF_PASS_LIST(LIST) PASS_LIST_NO_##LIST,
> > +enum pass_list
>
> An enum is a list? I understand this is nit-picking in moved existing
> code but I'd change it to something less misleading anyway. Is it
> impossible to keep the union name-less for some reason?
Is anyone actually using the pass lists?
> > +{
> > + GCC_PASS_LISTS
> > + PASS_LIST_NUM
> > +};
> > +#undef DEF_PASS_LIST
> > +
> > +namespace gcc {
> > +
> > +class context;
> > +
> > +class pipeline
> > +{
> > +public:
> > + pipeline(context *ctxt);
> > +
> > + void register_pass (struct register_pass_info *pass_info);
> > + void register_one_dump_file (struct opt_pass *pass);
> > +
> > + opt_pass *get_pass_for_id (int id) const;
> > +
> > + void dump_passes () const;
> > +
> > + void dump_profile_report () const;
> > +
> > +public:
>
> Extra public? Or do we want that to divide data members from methods?
I added it as a marker to separate methods from data members, but I
suspect I'm, ahem, "setting precedent" here.
http://gcc.gnu.org/codingconventions.html#Class_Form lists the order in
which things should be declared within a class.
> Thanks for all the work and sorry for the nagging. However, you might
> be setting up quite a few C++ precedents so I thought it would be
> better to pay attention :-)
Agreed - thanks for looking through the patch.
Dave