Update the coding standards to provide some guidance for a few constructs in C++11
Needs ReviewPublic

Press ? to show keyboard shortcuts.
Author
chandlerc
Reviewers
rsmith
doug.gregor
rjmccall
Repository
rL (LLVM)
Lint
Lint Skipped
Unit
Unit Tests Skipped
Apply Patch
arc patch D2905
Projects
None
Subscribers
Matt, silvas, lhames and 2 others
Summary

When looking at switching LLVM's code to build in C++11 mode, most of the coding standard applies just fine. Most of the new features we know how to use, or we will learn through code review. But a few of the more mechanical bits would be easy to diverge wildly and result in inconsistency and confusion. In some cases, there are features that are fairly important to be used in a particular way to keep the code maintainable.

I talked to Richard Smith, Doug Gregor, and John McCall (as some of the more language oriented folks in the community), and with their help I came up with these additions to the coding standards. I'd like to get some broader input on these, but ideally I'd like to paint these bikesheds pretty quickly, put something in place, and revisit them if these guidelines don't really work out in practice.

jdennett added a comment.Via LegacyFeb 28 2014, 7:57 PM

This looks helpful, I have just a couple of questions about how the sun might strike the southern aspect of the bikeshed in summer.

Inline Comments
docs/CodingStandards.rst
513

Meaning lambdas, or...? (I"m not sure this warrants any editing, but as a reader my first reaction to this was "I wonder what that's talking about".)

667

Does use of a constructor taking a std::initializer_list<T> parameter count as "notionally equivalent" to aggregate initialization, or is it under the general "Do not use these...if you care that you're calling some *particular* constructor" case? I could make an argument for either.

Maybe the easiest way to document that is with an example indicating whether
std::vector<int> Very SmallPrimes = { 2, 3, 5 };
is permitted or not.

(While I do have a preferred answer, I'm not here to lobby for it, just to ask what the intent is. Lobbying can happen later. Much later.)

lhames added a comment.Via LegacyFeb 28 2014, 11:04 PM

Seconding jdennett's comments.

Other than that, LGTM.

chandlerc added inline comments.Via LegacyMar 1 2014, 12:08 AM
Inline Comments
docs/CodingStandards.rst
513

Yep, just meaning lambdas.

667

Hmm. I'm a bit torn on this one.

I am OK allowing the variable syntax you give, but I would like to require that if you are creating a temporary, you do something like:

std::vector<int>({1, 2})

I would also be OK with requiring the same syntax for variable declarations:

std::vector<int> x({1, 2});
silvas added inline comments.Via LegacyMar 1 2014, 3:42 PM
Inline Comments
docs/CodingStandards.rst
490–502

Sort of random, but it seems like clang-format doesn't get this quite right. Maybe we should add something to the coding standard describing how to work around/with clang-format to get desirable results?

E.g.

First attempt, no coercion:

int foo() {
  dyn_switch(V->stripPointerCasts(), [](PHINode *PN) {
                                       // process phis...
                                     },
             [](SelectInst *SI) {
               // process selects...
             },
             [](LoadInst *LI) {
               // process loads...
             },
             [](AllocaInst *AI) {
    // process allocas...
  });
}

Adding one comment:

int foo() {
  dyn_switch(V->stripPointerCasts(), //
             [](PHINode *PN) {
               // process phis...
             },
             [](SelectInst *SI) {
               // process selects...
             },
             [](LoadInst *LI) {
               // process loads...
             },
             [](AllocaInst *AI) {
    // process allocas...
  });
}

And then, for the "hanging" last lambda:

int foo() {
  dyn_switch(V->stripPointerCasts(), //
             [](PHINode *PN) {
               // process phis...
             },
             [](SelectInst *SI) {
               // process selects...
             },
             [](LoadInst *LI) {
               // process loads...
             },
             [](AllocaInst *AI) {
               // process allocas...
             }//
             );
}

(The lack of space before that line comment is PR19017).

chandlerc added inline comments.Via LegacyMar 2 2014, 1:05 AM
Inline Comments
docs/CodingStandards.rst
490–502

Bleh. Lame. I've filed PR19021 for this. I think we should just fix clang-format here.

Matt added inline comments.Via LegacyMar 4 2014, 2:25 AM
Inline Comments
docs/CodingStandards.rst
667

Hi!

Out of curiosity, I'm wondering about the upsides/downsides of following the Scott Meyers' advice in this context.
// "Develop the habit of using brace initialization without “=“.
In: "Overview of the New C++ (C++11)"; http://www.aristeia.com/C++11.html

In "Uniform Initialization Syntax" he provides an example where the alternative would fail:

const float * pData = new const float[3] = { 1.f, 2.f, 3.f }; // error
const float * pData = new const float[3] { 1.f, 2.f, 3.f }; // ok

Similarly, another example shows that T var = expr syntax can’t call explicit constructors; adopting from the example (slightly simplifying, changing Widget to struct) therein:

struct Widget
{
	explicit Widget(int);
};
Widget w1(10); // okay, direct init: explicit ctor callable
Widget w2{10}; // ditto
Widget w3 = 10; // error! copy init: explicit ctor not callable
Widget w4 = {10}; // ditto

Or, is disallowing the above the actual intent?

Revision Update History

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 17441Feb 28 2014, 6:38 PM

Table of Contents

PathCoverage (All)Coverage (Touched)
Mdocs/CodingStandards.rst (128 lines)--

Diff 7441

docs/CodingStandards.rst

Loading...

Add Comment