[clang-format] Insert a space between a numeric UDL and a dot

[owenpan]

Summary

Fixes https://github.com/llvm/llvm-project/issues/60576.

[rymiel]

I've been sniped! (but only because i spent hours discussing with people if the error is pedantically valid, which it turns out it is)

[owenpan]

In D143546#4111827, @rymiel wrote:

I've been sniped! (but only because i spent hours discussing with people if the error is pedantically valid, which it turns out it is)

I was wondering why you didn't assign yourself. :) When I saw the bug report, I immediately remembered D134853#3829710 and knew it was indeed a bug. As this one is an invalid-code-generation bug, I wanted it fixed ASAP.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

3896

Add a comment what that is? Without the bug report I'd not know what that sequence would be.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

3896

I could do that, but the github issue is linked in the summary above and will be in the commit message. In general, I don't like unnecessary comments littered in the source. They can become outdated, out of place, misleading, and even wrong. How about giving an example as shown above?

[rymiel]

In D143546#4112077, @owenpan wrote:

As this one is an invalid-code-generation bug, I wanted it fixed ASAP.

Do you intend to backport it to the 16 release branch then?

[rsmith]

I wonder if this can be fixed more generally by using TokenConcatenation::AvoidConcat to determine whether clang-format should require a space between two tokens. This is the logic that clang -E uses when printing preprocessed tokens to avoid token splices. For example, that should also stop clang-format breaking the last line of this:

struct A;
void operator<=(A, A);
template<void(A, A)>
struct B {};
B<operator<= > b;

clang/lib/Format/TokenAnnotator.cpp

3896

Does clang-format have any formatting modes where it would leave out spaces around + or -? The same issue arises with things like 0xe + n, where removing the space between the 0xe and the + results in a token splice.

[owenpan]

In D143546#4113730, @rsmith wrote:

I wonder if this can be fixed more generally by using TokenConcatenation::AvoidConcat to determine whether clang-format should require a space between two tokens.

Probabtly not as clang-format uses the raw lexer that doesn't involve the preprocessor, which seems to be required by TokenConcatenation.

clang/lib/Format/TokenAnnotator.cpp

3896

No. I was aware of them and had made sure clang-format already handled them correctly.

[owenpan]

In D143546#4113721, @rymiel wrote:

In D143546#4112077, @owenpan wrote:

As this one is an invalid-code-generation bug, I wanted it fixed ASAP.

Do you intend to backport it to the 16 release branch then?

I don't know but will go with whatever you, @MyDeveloperDay and @HazardyKnusperkeks prefer.

[HazardyKnusperkeks]

In D143546#4114341, @owenpan wrote:

In D143546#4113721, @rymiel wrote:

In D143546#4112077, @owenpan wrote:

As this one is an invalid-code-generation bug, I wanted it fixed ASAP.

Do you intend to backport it to the 16 release branch then?

I don't know but will go with whatever you, @MyDeveloperDay and @HazardyKnusperkeks prefer.

I'd vote in favor, letting a code breaking bug knowingly in the most current (or to come? I don't follow that.) version would be let's say not nice.

clang/lib/Format/TokenAnnotator.cpp

3896

I could do that, but the github issue is linked in the summary above and will be in the commit message. In general, I don't like unnecessary comments littered in the source. They can become outdated, out of place, misleading, and even wrong. How about giving an example as shown above?

If you do a git blame you can come to the commit and thus to the bug, but if you are just reading, at least I don't blame all the lines. There I think a comment is nice, and your proposal is a really nice one.
But I've accepted the patch as is, and so the decision is yours. :)

[MyDeveloperDay]

LGTM

[MyDeveloperDay]

In D143546#4114341, @owenpan wrote:

In D143546#4113721, @rymiel wrote:

In D143546#4112077, @owenpan wrote:

As this one is an invalid-code-generation bug, I wanted it fixed ASAP.

Do you intend to backport it to the 16 release branch then?

I don't know but will go with whatever you, @MyDeveloperDay and @HazardyKnusperkeks prefer.

honestly, I trust your judgement @owenpan. I'm ok either way.

[owenpan]

In D143546#4116196, @HazardyKnusperkeks wrote:

In D143546#4114341, @owenpan wrote:

In D143546#4113721, @rymiel wrote:

In D143546#4112077, @owenpan wrote:

As this one is an invalid-code-generation bug, I wanted it fixed ASAP.

Do you intend to backport it to the 16 release branch then?

I don't know but will go with whatever you, @MyDeveloperDay and @HazardyKnusperkeks prefer.

I'd vote in favor, letting a code breaking bug knowingly in the most current (or to come? I don't follow that.) version would be let's say not nice.

+1.

clang/lib/Format/TokenAnnotator.cpp

3896

+1.

[owenpan]

In D143546#4116354, @MyDeveloperDay wrote:

In D143546#4114341, @owenpan wrote:

In D143546#4113721, @rymiel wrote:

In D143546#4112077, @owenpan wrote:

As this one is an invalid-code-generation bug, I wanted it fixed ASAP.

Do you intend to backport it to the 16 release branch then?

I don't know but will go with whatever you, @MyDeveloperDay and @HazardyKnusperkeks prefer.

honestly, I trust your judgement @owenpan. I'm ok either way.

Thanks! I've added the backport request.

clang/lib/Format/TokenAnnotator.cpp

if (Keywords.isWordLike(Right) && Keywords.isWordLike(Left))
     return true;
 
   // Leave a space between * and /* to avoid C4138 `comment end` found outside
   // of comment.
   if (Left.is(tok::star) && Right.is(tok::comment))
     return true;
 
   if (Style.isCpp()) {
+    // Space between UDL and dot: auto b = 4s .count();
+    if (Right.is(tok::period) && Left.is(tok::numeric_constant))
+      return true;
     // Space between import <iostream>.
     // or import .....;
     if (Left.is(Keywords.kw_import) && Right.isOneOf(tok::less, tok::ellipsis))
       return true;
     // Space between `module :` and `import :`.
     if (Left.isOneOf(Keywords.kw_module, Keywords.kw_import) &&
         Right.is(TT_ModulePartitionColon)) {
       return true;

clang/unittests/Format/FormatTest.cpp

 TEST_F(FormatTest, InsertNewlineAtEOF) {
   FormatStyle Style = getLLVMStyle();
   Style.InsertNewlineAtEOF = true;
 
   verifyFormat("int i;\n", Style);
   verifyFormat("int i;\n", "int i;", Style);
 }
 
+TEST_F(FormatTest, SpaceAfterUDL) {
+  verifyFormat("auto c = (4s).count();");
+  verifyFormat("auto x = 5s .count() == 5;");
+}
+
 } // namespace
 } // namespace format
 } // namespace clang