[clang-format] [C++20] [Module] clang-format couldn't recognize partitions

[Quuxplusone]

Bugzilla Link	PR52517
Status	CONFIRMED
Importance	P enhancement
Reported by	Chuanqi Xu (yedeng.yd@linux.alibaba.com)
Reported on	2021-11-16 00:28:31 -0800
Last modified on	2021-11-18 03:12:39 -0800
Version	trunk
Hardware	PC All
CC	blitzrakete@gmail.com, erik.pilkington@gmail.com, llvm-bugs@lists.llvm.org, mydeveloperday@gmail.com, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

The clang-format now coulnd't recognize module partitions correctly. It would format

export module foo:bar;

to

export module foo : bar;

[Quuxplusone]

So it thinks the ":" is an inheritance colon much like

public class Foo : Bar

hence the spacing? Should it be treated the same way do you think? i.e is it legal code once its been reformatted.

My preference I think is to make it so there is no space here and if someone request that there is then add an option SpaceBetweenModules?

What do you think?

[Quuxplusone]

(In reply to MyDeveloperDay from comment #1)
> So it thinks the ":" is an inheritance colon much like
>
> public class Foo : Bar
>
> hence the spacing? Should it be treated the same way do you think? i.e is it
> legal code once its been reformatted.
>
> My preference I think is to make it so there is no space here and if someone
> request that there is then add an option SpaceBetweenModules?
>
> What do you think?

Since partitions is not supported in trunk now, there is not a de facto
standard format. The reason why I think it is a bug since all the examples from
the standard don't have a space. And it is odd to me if there is a space. So I
think it should be good to add an option as you described.

(BTW, it looks like you are the maintainer of clang-format, could I CC you in
case I met clang-format problems?)

[Quuxplusone]

> (BTW, it looks like you are the maintainer of clang-format, could I CC you in
case I met clang-format problems?)

Not officially, but I have been working on it for a couple of years and added
the following capabilities

C#, Json support
East/West const fixer
Concepts
Plus numerous bug fixes.

I recognize your name as someone who is working on the later C++ 20/23 items, I
would like clang-format to try and be a little ahead of the game before people
start using them ;-) rather than playing catch up.

So absolutely copy me, and log items here or in github (when we go that way)
and I'll keep and eye out.

[Quuxplusone]

> Since partitions is not supported in trunk now, there is not a de facto
standard >format. The reason why I think it is a bug since all the examples
from the
> standard don't have a space. And it is odd to me if there is a space. So I
think > it should be good to add an option as you described.

I completely agree, I'm taking some examples from cppreference and adding unit
tests to cover those cases

If we are supposed to support a space then we can add that later. as

SpaceBetweenModuleParitions: true

but for now lets just ensure they don't get added in. (I'm seeing a couple of
other failure scenarios.

I'll send a review your way later.

[Quuxplusone]

> I would like clang-format to try and be a little ahead of the game before
> people start using them ;-) rather than playing catch up.

Yeah, that's the style all of us wanted. Before I have tried to contribute to
clang-format for coroutines, it is because the format test fails after we
introduce coroutines in our codes. So I am pushed to fix it. It would be very
appreciated if I know you that time. But it is not later now : )

>> I completely agree, I'm taking some examples from cppreference and adding
unit tests to cover those cases
>>
>> If we are supposed to support a space then we can add that later. as
>>
>> SpaceBetweenModuleParitions: true
>>
>> but for now lets just ensure they don't get added in. (I'm seeing a couple
of other failure scenarios.
>>
>> I'll send a review your way later.

Thanks!

[Quuxplusone]

Possible fix

https://reviews.llvm.org/D114151