friedmud commented 8 years ago

With C++11 there are several new language constructs. We need to decide how we want to format them.

The first three to tackle are: auto, range-based-for and lambda functions.

`auto`

When to use `auto`:

Return types:

I propose that we use auto for catching the return type of any function like so:

auto dof = elem->dof_number(0,0,0);
auto & data = getSomeData();

This has a few advantages:

It prevents mistakes (for instance, it prevents catching the above dof index as an incompatible type like unsigned int).
Allows for greater flexibility when refactoring (for instance, if getSomeData() were to start returning a const reference the above code would still work also: the above code would still work if the data type actually changed for getSomeData())

This will take some getting used to... but once you're used to it you will never go back.

Iterator based `for` loops

We should definitely use auto for all iterator based looping:

for (auto it = obj.begin(); it != obj.end(); ++it)

And definitely for range-based for loops:

for (auto & obj : container)

We should always prefer catching objects as auto & references to avoid copies. I haven't seen any breakdown on the speed of doing this for simple objects like std::vector<int> but I suspect that compilers will optimize that case properly no matter what we do.

When NOT to use `auto`:

Function return types

I think we should forbid using auto as the return type for functions (and similarly forbid the use of decltype for inferring return types). As a framework I think it's important that our header files clearly express the types that will be returned from functions... it's one of the main ways we "document" the library.

That said, there are some valid use-cases for this... and we can definitely have a discussion about it

`unsigned int` `for` loops

I think that we should still be explicit when we really want unsigned int based loops:

for (unsigned int i = 0; i < 4; i++)

The reason for this is that by default auto i would be an int instead of unsigned int. To get an unsigned int we would need to do

for (auto i = 0u; i < 4; i++)

Which is just awkward.

Range-based `for`

One of the best new things in C++11. Here’s my proposal for how most loops should look:

for (auto & obj : container)

As mentioned above... we should prefer to catch objects as references. Also: I think it’s a good idea to put space around the : (colon).

Lambdas

Lambdas are awesome... we’re going to end up with my of them all over the code, so it’s important to nail them down. Also: they have some funky syntax…

Here is my current proposal for passing lambdas as arguments:

Threads::parallel_for(elem_range,
                      [&](const ElemRange & elem_range)
                      {
                        // Save this off
                        auto block_map_end = _block_map.end();

                        for (auto & elem : elem_range)
                        {
                          auto & sid = elem->subdomain_id();

                          auto entry = _block_map.find(sid);

                          if (entry != block_map_end)
                            sid = entry->second;
                        }
                      });

The important thing to remember here is that the lambda is an argument to parallel_for()... so it’s indented like one. This also happens to be the way Emacs wants to space out lambdas, so that’s a bonus!

The curly braces on their own lines just like our guidelines say for all functions... and they line up with each other.

The only weirdness is what to do with the trailing parenthesis and semicolon. Those are closing the argument list for parallel_for() and ending the parallel_for() statement. They could be on their own line. I chose to put them this way because they look awkward on their own line. But I can see the argument for the parenthesis going on it’s own line in line with the opening parenthesis and the semicolon just after it.

I don’t see any reason to put spaces around the & in the “capture type” list ([&]). Having it there just in front of the argument list for the lambda makes it kind of just look like a regular function (almost like that’s the function name). I do think we should use “implicit capture by reference” like this most of the time for getting data into a lambda (unless, of course, you have a good reason for something else).

If you’re going to declare a lambda as a variable you should use auto like this:

auto subdomain_lambda =
  [&](const ElemRange & elem_range)
  {
    // Save this off
    auto block_map_end = _block_map.end();

    for (auto & elem : elem_range)
    {
      auto & sid = elem->subdomain_id();

      auto entry = _block_map.find(sid);

      if (entry != block_map_end)
        sid = entry->second;
    }
  };

To be clear: that’s two spaces to indent the lambda. Again, I put the semicolon on the same line with the ending curly-brace for the lambda. In this case I think it would look really weird on a line by itself.

The other option here is that you could start the lambda after the = sign and then indent it all over so it lines up. Firstly, Emacs will fight you tooth and nail if you try to do this. Secondly... there is no need for it.

If the lambda is sufficiently short I think we can put it all on one line:

auto short_lambda = [](){ return 4; };

Note that I didn’t specify a return value. In spite of what I said earlier about specifying return values... lambdas are different because they are “local"... they are not part of the “API” so it’s ok to just let the compiler infer the return type. In cases where you do want to specify the return type I propose this:

auto short_lambda = []() -> int { return 4; };

Spaces before and after the arrow operator and after the type help with readability I think.

jwpeterson commented 8 years ago

This sounds good to me.

I will have to get used to catching the return types of every function call as auto... seems like it could get confusing if there are several "layers" of it, e.g.

auto var1 = func1();
auto var2 = func2(var1);
auto var3 = func3(var1, var2);

In case func3 has several overloads, you might have to think for a minute about which one is actually called...

permcody commented 8 years ago

@jwpeterson is right - let's not sweep through the framework and change all caller returns to auto. There are times when using an explicit type is a good idea. These nested returns are one of those cases. We just need to use common sense. It's somewhat analogous to the case where you split an expression into into multiple statements because they get unwieldy. I agree with the rest of @friedmud's suggestions on auto especially in the case of literals.

I do have a few opinions on lambdas. I think they are great for some things like replacing simple functors but they don't necessarily convey great structure to the framework. If we have a new framework level thread loop, we should probably create a full-fledged class instead of stuffing it all into a lambda. Yeah, that's the other extreme but again, common sense. They can be difficult to grasp conceptually so they probably shouldn't be exposed to users.

The parallel-for loop thing is handy. We'll have to be careful about introducing these things in threaded regions just because they are easy to use!

friedmud commented 8 years ago

I agree with @permcody . If you think an explicit type is needed... then do it.

I also agree about lambdas. @permcody , what you'll actually find out is that there are several limitations of lambdas as threaded functors. The most obvious one: they don't work for reductions! Our reductions require a join() method which cannot be done with lambdas. In the future we could change our reductions to accept two lambdas (body and join)... or we could operate on thread-local data.... and join afterward... but that's not important right now.

Because of this (and other things that are just easier to do with a full Functor class) we'll definitely have full *Loop classes for most of our stuff. These are just really handy for one-off, simple loops.

friedmud commented 8 years ago

BTW: If we're using OpenMP or TBB there is no problem with us nesting threaded loops inside of each-other (the thread pools will take care of everything). However... pthreads could HAMMER your machine if you do that ;-)

jwpeterson commented 8 years ago

On Tue, May 17, 2016 at 12:18 PM, Daniel Schwen notifications@github.com wrote:

One thing about range based for. Instead of

for (auto & obj : container)

we should probably straight up encourage

for (auto && obj : container)

I disagree, based on the accepted answer here:

http://stackoverflow.com/questions/13130708/what-is-the-advantage-of-using-universal-references-in-range-based-for-loops

John

friedmud commented 8 years ago

I actually agree that we should prefer auto && (not sure where @dschwen's comment went)

dschwen commented 8 years ago

I deleted it. I changed my mind, as the universal reference would be yet another alien concept that we'd introduce.

On Tue, May 17, 2016, 12:41 PM Derek Gaston notifications@github.com wrote:

I actually agree that we should prefer auto && (not sure where @dschwen https://github.com/dschwen's comment went)

— You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub https://github.com/idaholab/moose/issues/6993#issuecomment-219813647

jwpeterson commented 8 years ago

👽

friedmud commented 8 years ago

(Please don't delete comments! This is a discussion!)

All of this is alien. People can learn. We need to decide what we prefer.

The reason I didn't put universal references in my recommendations is simply because I'm still unfamiliar with them. I've used them some (where it was necessary)... but I need to use them more.

I think we should prefer them because they are more general... and will help keep us from changing code as the framework is refactored.

permcody commented 8 years ago

Currently writing my first lambda. I have one small tweak to Derek's proposal. Should we consider putting the closing parenthesis on a separate line instead of immediately after the lambda body?

some_method(stuff, 
            [&](int param)
            {
               ...
            }
           );

I guess it doesn't really matter to me, just thinking about it.

friedmud commented 8 years ago

I'm ok either way... I've tried it both.

Having it on it's own line is maybe a bit clearer about what's going on...

On Fri, May 27, 2016 at 1:43 PM Cody Permann notifications@github.com wrote:

Currently writing my first lambda. I have one small tweak to Derek's proposal. Should we consider putting the closing parenthesis on a separate line instead of immediately after the lambda body?

some_method(stuff, [&](int param) { ... } );

I guess it doesn't really matter to me, just thinking about it.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/6993#issuecomment-222209552, or mute the thread https://github.com/notifications/unsubscribe/AA1JMURicRG8beNE9vl-OjzEeR2JOmvZks5qFy1EgaJpZM4IgXiL .

permcody commented 8 years ago

Closing this ticket, I think we've capture the C++11 syntax on our wiki and are slowing starting to use it.

friedmud commented 8 years ago

Umm... where?

permcody commented 8 years ago

I just updated the code standards on the wiki. Check them out and make sure they look good to you then close this ticket. Feel free to edit.

friedmud commented 8 years ago

Looks good!

idaholab / moose

C++11 Code Standards #6993

`auto`

When to use `auto`:

Return types:

Iterator based `for` loops

When NOT to use `auto`:

Function return types

`unsigned int` `for` loops

Range-based `for`

Lambdas

idaholab / moose

C++11 Code Standards #6993

auto

When to use auto:

Return types:

Iterator based for loops

When NOT to use auto:

Function return types

unsigned int for loops

Range-based for

Lambdas

`auto`

When to use `auto`:

Iterator based `for` loops

When NOT to use `auto`:

`unsigned int` `for` loops

Range-based `for`