Example of misformatting with the current clang-format

[sbastrakov]

This piece in PIConGPU seems to have been misunderstood and therefore misformatted by clang-format. I am not sure exactly why.

Admittedly, it is rather (and likely too) complicated anyhow. What is going on there:

• ForEachIdx<IdxConfig<numCellsPerSuperCell, numWorkers>> is a functor type
• We instantiate an object of this type by passing workerIdx to its constructor, so the curly brace at the end of line 398 is call for constructor, not start of a block
• Then we immediately call operator() of the freshly constructed object in the same expression
• And pass a lambda defined just there as its argument

Edit: so what the code with the current formatting boils down to (line 398 and start of line 399) is

Type{
     constructor parameter}( lots of other stuff ...

and I suspect the other stuff was complicated and it did not recognize the constructor parameter.

cc @j-stephan

[bernhardmgruber]

The formatting looks correct to me. What is incorrectly formatted?

Although you I see that you could come up with arguments why the nested lambda should be indented twice.

[sbastrakov]

Well, it formatted as

Type{
     constructor parameter}(...

(line 398 and start of line 399), I do not think it is correct.

[psychocoderHPC]

What is the result if we use () for the constructor?

[j-stephan]

I'm not sure what the preferable way of formatting should look like.

The current way isn't pretty:

ForEachIdx<IdxConfig<numCellsPerSuperCell, numWorkers>>{
    workerIdx}([&](uint32_t const linearIdx, uint32_t const) {

But this is even worse IMO:

ForEachIdx<IdxConfig<numCellsPerSuperCell, numWorkers>>{workerIdx}
    ([&](uint32_t const linearIdx, uint32_t const) {

We could also break somewhere in the lambda parameter list but that is awful, too. I guess this is just one of those cases where there is no good solution.

[sbastrakov]

I agree the code is just very complicated and that's the main problem. But how it got formatted is in my opinion not just visually iffy, but more importantly actively misleading, as from the first look it seems to have a new code starting, which is not the case,

[j-stephan]

Would it be possible to have an ForEachIdx object? This way it could be broken into two lines:

auto loop = ForEachIdx<IdxConfig<numCellsPerSuperCell, numWorkers>>{workerIdx};
loop([&](uint32_t const linearIdx, uint32_t const)
{
    /* body goes here */
});

[sbastrakov]

Yes, I think that would have been better to do in the first place.