Allow trailing coma in asserts

[deadalnix]

Because

[Hackerpilot]

This is both ugly and consistent with the way that function argument/parameter parsing works...

[yebblies]

I guess this is next?

if (x,)
{
}

[deadalnix]

No

[WalterBright]

Because

Need a more compelling justification.

[deadalnix]

@WalterBright Simply the ability to have consistent formatting when passing a series of argument to something.

[AndrejMitrovic]

It's a little early for april fools!

@WalterBright Simply the ability to have consistent formatting when passing a series of argument to something.

If you're talking about mixins you can use array.join(",");, though you might have to change a foreach loop into something simpler.

[deadalnix]

I'm not talking about mixin. If I was talking about mixin I would have used the word mixin. I'm talking about being able to format argument list consistently. This has been explained both by myself and @Hackerpilot , if you have something to say about this, please do, but if you plan to poison the well by using dismiss terms like "april fool" make sure to do it in a way one doesn't have to question your ability to pass the Turing test.

[AndrejMitrovic]

Take it easy..

Anyway if it's consistent with everything else DMD does then maybe it should be allowed indeed.

[deadalnix]

No hard feelings.

[Hackerpilot]

@Hackerpilot , if you have something to say about this, please do, but if you plan to poison the well by using dismiss terms like "april fool" make sure to do it in a way one doesn't have to question your ability to pass the Turing test.

Wrong person.

[deadalnix]

There is a comma.

This has been explained both by myself and @Hackerpilot , ...

[dnadlinger]

@deadalnix: I feel like I'm missing some backstory here. Why do you insist on staying so vague instead of writing a proper explanatory commit message/justification as I'm sure you would do normally?

Anyway, it's true that DMD allows trailing commas in other argument lists (function calls, struct literals), so it is indeed consistent in a sense. Not sure whether the better "fix" isn't to just disallow it in the other case, though.

[deadalnix]

Trailing comma are useful. I do not really wish to go back on this argument, it has been debated to death already in all languages and file format that exists. There are good usability reasons and blame conservation reasons.

D already has trailing comma and it is for the best. This is about removing inconsistency.

[yebblies]

Trailing comma are useful. I do not really wish to go back on this argument, it has been debated to death already in all languages and file format that exists. There are good usability reasons and blame conservation reasons.

I understand why a trailing comma is useful in enum declarations, function argument lists, array literals etc. But assert is not a function, and does not have variadic arguments.

My though is that this is good style:

int[] x =
[
    1,
    2,
    3,
    4,
];

but this is not:

assert(x, "message",);

I'm expecting the first would be preferred, and the latter would be rejected during code review.

If I'm not the only one that thinks this, then it would seem we're adding support for a syntax that should never be used.

So, do any of the "good usability reasons and blame conservation reasons" actually apply to assert?

[deadalnix]

@yebblies , both at work and in various projects of mine, I use trailing comma for multiple lines argument lists, and no trailing comma for single line ones.

foo(a, b,) // bad style
foo(
  a,
  b,
) // good style.

It is relevent when a and b are actually long expressions rather than single letters.

While assert is not a function it is to be noted we do this for all argument lists in D, not only function's. We do it for array lterals, for template arguments, for templateand function parameters, and more. Assert is the exception. I do not wish to special case assert formating when condition/message are long.

More generally, I think it is not good to allow or disallow trailing comma, or prety much anything else on a case by case basis. That makes the language more complex for no good benefit.

[AndrejMitrovic]

No hard feelings.

FYI I've always admired you @deadalnix and it really felt like your comment came out of left-field. I'm sorry if my own comment made it look like I had bad intentions.

I can understand the frustration with making DMD pulls which leads to one to being pessimistic (been there, done that..), so let's just say fuck it and bury the hatchets?

I agree that we really need to keep these rules consistent. The more special cases involved the more things we need to keep in our head.

Btw @deadalnix, is there any way we can make this implementation generic instead of specializing for TOKassert? Question goes to @9rnsr too.

[deadalnix]

@AndrejMitrovic Yes, let's burry the hatchet. Sorry for being hot blooded.

As to make this more generic, I don't know much what it involve. I'm only somewhat familiar with DMD source code. if someone more experienced with DMD suggest a way to make this more generic, I'd be happy to do it.

[AndrejMitrovic]

@AndrejMitrovic Yes, let's burry the hatchet. Sorry for being hot blooded.

Cool. :)

if someone more experienced with DMD suggest a way to make this more generic, I'd be happy to do it.

Don't worry about it too much, we can live with the special case for now (obviously it was never generic to begin with). Just make it go green so we can merge it.

Btw, anyone know what Kenji is up to? Apparently he hasn't had any activity in over 6 months. Hope he's just busy with other things now. :)

[deadalnix]

Updated, that should be green soon.

[deadalnix]

Ping ?

[Hackerpilot]

To clarify my position: I don't like trailing commas in argument lists, but that doesn't matter because we already have them. I am in favor of this change because consistency is important.

[9rnsr]

In dmd code style, if-condition and its body are placed in their own lines.

[9rnsr]

I've come back.

is there any way we can make this implementation generic instead of specializing for TOKassert?

Currently parseParameters wouldn't work well against the fixed-number arguments. So making implementation more generic would need more work. In other words, this PR is enough simple as the new behavior implementation.

LGTM.

[deadalnix]

Done, welcome back @9rnsr !

[9rnsr]

LGTM. I'm on a positive stance to the change. When an assert code is generated by string mixin, reducing special cases for a trailing comma would be useful.

[AndrejMitrovic]

Auto-merge toggled on

[deadalnix]

<3