Page MenuHomePhabricator

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

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

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=52517

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;

to

import<ctime>;
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;
template<...>
export using A = B<T>;
export {

};
export type_name var;

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

Thanks!

clang/docs/ReleaseNotes.rst
264
clang/unittests/Format/FormatTest.cpp
22601

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.

22606

It lacks a test for import "header";

MyDeveloperDay marked 3 inline comments as done.

Add some more tests

Quuxplusone added inline comments.
clang/unittests/Format/FormatTest.cpp
22638

I'd like to see module :private;
I'd like to see import <foo/bar.h>;
I'd like to see import foo...bar;
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;
import..........;
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
clang/unittests/Format/FormatTest.cpp
22638

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
clang/lib/Format/TokenAnnotator.cpp
3265

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

3269–3270

You can drop !Left.is(Keywords.kw_import) 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.

3274

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

3277

You can fold this line into Line 3223 above.

clang/lib/Format/UnwrappedLineParser.cpp
1344

Maybe move this entire block into a function?

1351

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

1353–1355

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

1358–1359

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

Quuxplusone added inline comments.Fri, Nov 19, 7:49 AM
clang/lib/Format/TokenAnnotator.cpp
3274

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
clang/lib/Format/TokenAnnotator.cpp
3265

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
clang/lib/Format/UnwrappedLineParser.cpp
1119–1141

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.
clang/lib/Format/UnwrappedLineParser.cpp
1119–1141

won't FormatTok become null at eof?

MyDeveloperDay added inline comments.Mon, Nov 22, 3:00 PM
clang/lib/Format/UnwrappedLineParser.cpp
1119–1141

ok I see this..

Fix infinite loop

owenpan added inline comments.Tue, Nov 23, 7:08 PM
clang/lib/Format/UnwrappedLineParser.cpp
1119

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

1126–1127

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