Re: C++ coding conventions: namespaces, references and getters (was Re: [PATCH 2/2] Introduce beginnings of a pipeline class.)

[Gabriel Dos Reis <gdr at integrable-solutions dot net>]

[ Adding Benjamin, Diego, Lawrence ]

General remarks first:
When we designed the coding standards for GCC, an overriding
philosophy was that we did not want to be prescriptive.  Rather, we
explicitly wanted to encourage common sense and trust the judgment
of maintainers to make sound and appropriate judgments.

More than a year later, I still believe that was the right thing to do
and I would not want us to start being prescriptive.

On Mon, Jul 29, 2013 at 1:20 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> 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.

It is a "common wisdom" among C++ programmers that using directives
in header files generally negate the benefits of namespaces, and can
lead to surprises.  This is because most of the time, people don't go
scrutinize header files; they include them (based on documentation
of declared signatures) in many translation units.

There is one notable exception to this, which is known as
"namespace composition":

  http://stackoverflow.com/questions/16871390/why-is-namespace-composition-so-rarely-used

We should probably amend the coding standards and include that hint,
but I don't feel strongly about it.

>
> 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).

The using declarations are fine, as indicated.

>
> 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.

Agreed.

[…]

> 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,

I hope nobody is objecting to std::swap for example.

> 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?

I think we should use them where appropriate, and trust maintainers
of specific areas of the compiler to exercise sound judgments instead
of us trying to cast ideology into stones.

>
> 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.

Because, there shouldn't be.  They are part of basic
C++ programming techniques.

> Are C++ references permissible inside GCC's own code (outside of
> libstdc++, of course) and is the above usage sane?

Sound usage should be freely used.

> There is another question here, which is how people feel about
> accessors/getters?

Instead of general blanket rules,  I think the decision should be based
on specific abstractions and usage, not mechanical rules.   There are
places where I see getters/setters as poor abstraction and insufficient
thought applied (an example would be writing setters/getters for std::pair),
and other times where it reflects sound and brilliant abstraction.

> 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?)

Like I said, it depends on the abstraction and effects desired :-)

-- Gaby