New user self-registration is disabled due to spam. For an account please email bugs-admin@lists.llvm.org with your e-mail address and full name.

Bug 47589 - clang-format-11: regression in aligned comments
Summary: clang-format-11: regression in aligned comments
Status: RESOLVED FIXED
Alias: None
Product: clang
Classification: Unclassified
Component: Formatter (show other bugs)
Version: unspecified
Hardware: PC Linux
: P normal
Assignee: Unassigned Clang Bugs
URL:
Keywords:
Depends on:
Blocks: release-11.0.1
  Show dependency tree
 
Reported: 2020-09-20 03:11 PDT by Sylvestre Ledru
Modified: 2020-11-25 23:32 PST (History)
10 users (show)

See Also:
Fixed By Commit(s): b9e789447f14c0330edd22c82746af29e7c3b259 f590845f501


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sylvestre Ledru 2020-09-20 03:11:45 PDT
With this simple code:
----
namespace mozilla {

#define IPV6_SCOPE_GLOBAL 0      // Global scope.
#define IPV6_SCOPE_LINKLOCAL 1   // Link-local scope.
#define IPV6_SCOPE_SITELOCAL 2   // Site-local scope (deprecated).
#define IPV6_SCOPE_UNIQUELOCAL 3 // Unique local
#define IPV6_SCOPE_NODELOCAL 4   // Loopback

} // namespace mozilla
----

clang-format-10 left it intact

With clang-format-11, the output is:
----
% clang-format-11 IPv6Utils.h
namespace mozilla {

#define IPV6_SCOPE_GLOBAL 0 // Global scope.
#define IPV6_SCOPE_LINKLOCAL 1 // Link-local scope.
#define IPV6_SCOPE_SITELOCAL 2 // Site-local scope (deprecated).
#define IPV6_SCOPE_UNIQUELOCAL 3 // Unique local
#define IPV6_SCOPE_NODELOCAL 4 // Loopback

} // namespace mozilla
----

which is a significant regression for clang-format-11

Looking at the 11 release notes, https://prereleases.llvm.org/11.0.0/rc2/tools/clang/docs/ReleaseNotes.html
I haven't seen anything directly related.
Comment 1 MyDeveloperDay 2020-09-21 02:35:47 PDT
It as if AlignTrailingComments is not working on preprocessor output, it is working but not on the #defines

namespace mozilla {

#define IPV6_SCOPE_GLOBAL 0 // Global scope.
#define IPV6_SCOPE_LINKLOCAL 1 // Link-local scope.
#define IPV6_SCOPE_SITELOCAL 2 // Site-local scope (deprecated).
#define IPV6_SCOPE_UNIQUELOCAL 3 // Unique local
#define IPV6_SCOPE_NODELOCAL 4 // Loopback

int a = 0;   // a of course
int abc = 0; // abc of course

} // namespace mozilla
Comment 2 MyDeveloperDay 2020-09-21 02:38:36 PDT
I have a build from 26th May 2020 and its broken then... Do you have a working version with a SHA so I can narrow down between which commits it failed?
Comment 3 MyDeveloperDay 2020-09-21 06:40:33 PDT
This issue is caused by https://reviews.llvm.org/D79388, if I revert just this commit then the result is

namespace mozilla {

#define IPV6_SCOPE_GLOBAL 0      // Global scope.
#define IPV6_SCOPE_LINKLOCAL 1   // Link-local scope.
#define IPV6_SCOPE_SITELOCAL 2   // Site-local scope (deprecated).
#define IPV6_SCOPE_UNIQUELOCAL 3 // Unique local
#define IPV6_SCOPE_NODELOCAL 4   // Loopback

int a = 0;   // a of course
int abc = 0; // abc of course

} // namespace mozilla
Comment 4 Sylvestre Ledru 2020-09-21 07:31:28 PDT
Do you think we should revert this patch for the 11 release?
Comment 5 Marcus Johnson 2020-09-21 13:54:00 PDT
(In reply to MyDeveloperDay from comment #3)
> This issue is caused by https://reviews.llvm.org/D79388

That's great that you found a solution @MyDeveloperDay, I was nervous that my first LLVM patch broke something afterall haha.
Comment 6 Sylvestre Ledru 2020-09-21 23:18:53 PDT
> I was nervous that my first LLVM patch broke something afterall haha.

Don't be. clang-format should have a test for this!
Comment 7 Hans Wennborg 2020-09-22 02:54:41 PDT
Sorry, this is coming late so I don't think we can treat it as a blocker for 11.
Comment 8 SedatDilek 2020-09-22 05:18:45 PDT
I can confirm the revert of...

commit b2eb43931757 ("[clang-format] Fix AlignConsecutive on PP blocks")

...fixes the issue for me.

$ cat IPv6Utils.h
// Link: https://bugs.llvm.org/show_bug.cgi?id=47589
namespace mozilla {

#define IPV6_SCOPE_GLOBAL 0      // Global scope.
#define IPV6_SCOPE_LINKLOCAL 1   // Link-local scope.
#define IPV6_SCOPE_SITELOCAL 2   // Site-local scope (deprecated).
#define IPV6_SCOPE_UNIQUELOCAL 3 // Unique local
#define IPV6_SCOPE_NODELOCAL 4   // Loopback

} // namespace mozilla

$ which clang-format
/home/dileks/src/llvm-toolchain/install/bin/clang-format

$ clang-format ./IPv6Utils.h
// Link: https://bugs.llvm.org/show_bug.cgi?id=47589
namespace mozilla {

#define IPV6_SCOPE_GLOBAL 0      // Global scope.
#define IPV6_SCOPE_LINKLOCAL 1   // Link-local scope.
#define IPV6_SCOPE_SITELOCAL 2   // Site-local scope (deprecated).
#define IPV6_SCOPE_UNIQUELOCAL 3 // Unique local
#define IPV6_SCOPE_NODELOCAL 4   // Loopback

} // namespace mozilla

Note: I have cherry-picked the 4 commits from [1]. In the meantime Hans has done this for release/11.x Git.

[1] https://github.com/ClangBuiltLinux/linux/issues/1136#issuecomment-695162766
Comment 9 SedatDilek 2020-09-24 07:03:06 PDT
With LLVM toolchain version 11.0.0-rc3 this looks like reported (not fixed).

$ clang-format --version
clang-format version 11.0.0 (https://github.com/llvm/llvm-project 0b56e5490dc33e4e7a4fdd837e642f72a2659189)

$ clang-format ./IPv6Utils.h 
// Link: https://bugs.llvm.org/show_bug.cgi?id=47589
namespace mozilla {

#define IPV6_SCOPE_GLOBAL 0 // Global scope.
#define IPV6_SCOPE_LINKLOCAL 1 // Link-local scope.
#define IPV6_SCOPE_SITELOCAL 2 // Site-local scope (deprecated).
#define IPV6_SCOPE_UNIQUELOCAL 3 // Unique local
#define IPV6_SCOPE_NODELOCAL 4 // Loopback

} // namespace mozilla
Comment 10 Hans Wennborg 2020-09-24 07:30:06 PDT
Right. If there's a fix and we do an rc4, it could be merged, but I'm not holding the release for it.
Comment 11 Sylvestre Ledru 2020-10-19 05:29:09 PDT
@Hans, I pushed a revert in master.
Can I cherry-pick:
https://github.com/llvm/llvm-project/commit/b9e789447f14c0330edd22c82746af29e7c3b259
to 11? (or you want to do it ?
Comment 12 Sylvestre Ledru 2020-10-19 05:29:24 PDT
I also took the patch in Debian & Ubuntu packages.
Comment 13 Hans Wennborg 2020-10-19 05:31:29 PDT
(In reply to Sylvestre Ledru from comment #11)
> @Hans, I pushed a revert in master.
> Can I cherry-pick:
> https://github.com/llvm/llvm-project/commit/
> b9e789447f14c0330edd22c82746af29e7c3b259
> to 11? (or you want to do it ?

It's for Tom to decide.
Comment 14 Tom Stellard 2020-10-28 17:09:33 PDT
The fix does not apply cleanly, could someone backport this and push a branch to their local github fork?
Comment 16 Tom Stellard 2020-11-25 22:53:32 PST
Merged: f590845f501
Comment 17 Sylvestre Ledru 2020-11-25 23:32:32 PST
Merci!