Page MenuHomePhabricator

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

Authored by MyDeveloperDay on Thu, Nov 18, 3:12 AM.



clang-format is butchering modules, this could easily become a barrier to entry for modules given clang-formats wide spread use.

Prevent the following from adding spaces around the : (cf was considering the ':' as an InheritanceColon)

import <ctime>;
import foo:bar;
import :bar;
export module foo:bar;
export import foo:bar;
export import :bar;
module foo:bar;


import foo : bar;
import : bar;
export module foo : bar;
export import foo : bar;
export import : bar;
module foo : bar;

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Thu, Nov 18, 3:12 AM
MyDeveloperDay requested review of this revision.Thu, Nov 18, 3:12 AM

There could be more tests in case we want support c++20 module more. This shouldn't be a break issue for this patch since this aims to fix the space in partitions only.

export template<...>;
export struct {
export int func() {
   // ...
export using A = B;
export using A = B<T>;
export {

export type_name var;

Feel free to ignore this if you want to support them in later patches.



The module name could contain dot '.' in the middle. Although it seems like wouldn't be broken by clang-format, I think it is better to test it.


It lacks a test for import "header";

MyDeveloperDay marked 3 inline comments as done.

Add some more tests

Quuxplusone added inline comments.

I'd like to see module :private;
I'd like to see import <foo/bar.h>;
I'd like to see import;
I'd like to see import ..........;
Is module foo:private; a thing?
Is module public:while; a thing?

Nasty, but let me take a look

module: private;
import <foo / bar.h>;
import foo... bar;
module foo: private;
module public : while;

they all feel all kinds of wrong!

The current goal seems to be achieved. I have no idea what are acceptable module names or so, I've not yet looked into them.

This revision is now accepted and ready to land.Thu, Nov 18, 12:41 PM
Quuxplusone added inline comments.Thu, Nov 18, 2:06 PM

I don't intend to block this PR waiting on fixing these, but please at least add the "I'd like to see" test cases with TODO comments. That way, whoever tackles it will have the test cases already written.

As usual, I'm ambivalent about the "Is X a thing?" test cases — they arguably don't matter but arguably should be tested anyway to make sure clang-format doesn't crash on them.

ChuanqiXu accepted this revision.Thu, Nov 18, 6:08 PM

LGTM with @Quuxplusone's opinions addressed.

Add yet more unit tests and cover those.

Mark one test as TODO as I'm not sure if its allowed syntax

MyDeveloperDay marked 2 inline comments as done.Fri, Nov 19, 1:37 AM
owenpan added inline comments.Fri, Nov 19, 7:26 AM

You can remove kw_export as it must be followed by import or module, based on how TT_ModulePartitionColon is set on Line 908.


You can drop ! as import :bar is already covered by Line 3226. Also, I would remove kw_public and kw_private as they are not special WRT other keywords when followed by TT_ModulePartitionColon.


Is module :public a thing in C++20? If not, I would remove kw_public.


You can fold this line into Line 3223 above.


Maybe move this entire block into a function?


You can change this line to else if (FormatTok->is(tok::less)) { as FormatTok can't be null.


You can combine these three lines into one: while (FormatTok && FormatTok->isNot(tok::semi)) {


Don't you want to mark up to the matching >?

Quuxplusone added inline comments.Fri, Nov 19, 7:49 AM

For the record, my understanding is that module :public is not a thing, and neither is module public:while or any other combination of keywords except for module :private. (I don't even think module foo:private is a thing; is it?)
So +1 to removing kw_public. However, if the codebase happens to already have a function for isIdentifierOrKeyword, I think this would be a perfect place to use it. Consider an autoformatter-as-you-type dealing with module foo:public[X]_methods or module :if[X]stream.

ChuanqiXu added inline comments.Sun, Nov 21, 6:02 PM

export could be followed by many other declarations. (Maybe the logic on line 908 is not right, I am not sure)

MyDeveloperDay marked 10 inline comments as done.

Address review comments

owenpan added inline comments.Mon, Nov 22, 8:17 AM

Possible infinite loops if the import statement is the last line and not terminated by a ;?

MyDeveloperDay marked an inline comment as done.Mon, Nov 22, 2:59 PM
MyDeveloperDay added inline comments.

won't FormatTok become null at eof?

MyDeveloperDay added inline comments.Mon, Nov 22, 3:00 PM

ok I see this..

Fix infinite loop

owenpan added inline comments.Tue, Nov 23, 7:08 PM

A while (!eof()) or while (FormatTok->isNot(tok::eof) would be safer here just in case the last line is import followed by eof.


Possible infinite loop here as well. Maybe change it to while (!FormatTok->isOneOf(tok::greater, tok::semi, tok::eof) {?

MyDeveloperDay marked 3 inline comments as done.

Address review comments, guard against eof

owenpan accepted this revision.Thu, Nov 25, 3:36 AM