Fix override keyword being print to the left side

[giulianobelinassi]

Previously, the override keyword in C++ was being print in the left side of a method decl, which is unsupported by C++ standard. This commit fixes that by setting the CanPrintOnLeft field to 0, forcing it to be print on the right side of the decl.

This comes from the fact that #87281 won't be backported into clang-18. However since classes with override keyword is broken in LLVM-18, I think this is still worth fixing.

@vgvassilev Please check if it works in your test cases.

Notice that this PR targets release/18.x, and not the main branch.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Giuliano Belinassi (giulianobelinassi)

Changes

Previously, the override keyword in C++ was being print in the left side of a method decl, which is unsupported by C++ standard. This commit fixes that by setting the CanPrintOnLeft field to 0, forcing it to be print on the right side of the decl.

This comes from the fact that #87281 won't be backported into clang-18. However since classes with override keyword is broken in LLVM-18, I think this is still worth fixing.

@vgvassilev Please check if it works in your test cases.

Notice that this PR targets release/18.x, and not the main branch.

---

Full diff: https://github.com/llvm/llvm-project/pull/88453.diff

2 Files Affected:

• (modified) clang/include/clang/Basic/Attr.td (+2)
• (added) clang/test/AST/ast-dump-override-final.cpp (+20)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 58838b01b4fd7c..dbf2dd2120fb69 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1590,6 +1590,7 @@ def RegCall : DeclOrTypeAttr {
 }
 
 def Final : InheritableAttr {
+  let CanPrintOnLeft = 0;
   let Spellings = [CustomKeyword<"final">, CustomKeyword<"sealed">];
   let Accessors = [Accessor<"isSpelledAsSealed", [CustomKeyword<"sealed">]>];
   let SemaHandler = 0;
@@ -2472,6 +2473,7 @@ def Overloadable : Attr {
 }
 
 def Override : InheritableAttr {
+  let CanPrintOnLeft = 0;
   let Spellings = [CustomKeyword<"override">];
   let SemaHandler = 0;
   // Omitted from docs, since this is language syntax, not an attribute, as far
diff --git a/clang/test/AST/ast-dump-override-final.cpp b/clang/test/AST/ast-dump-override-final.cpp
new file mode 100644
index 00000000000000..c1cee6b01565f6
--- /dev/null
+++ b/clang/test/AST/ast-dump-override-final.cpp
@@ -0,0 +1,20 @@
+// This file contain tests to check if override and final are dumped in the
+// correct positions.
+
+// RUN: %clang_cc1 -ast-print -x c++ %s -o - | FileCheck %s
+
+// CHECK: class A {
+class A {
+  // CHECK-NEXT: virtual void f();
+  virtual void f();
+
+  // CHECK-NEXT: virtual void g() final;
+  virtual void g() final;
+} AA;
+
+// CHECK: class B : public A {
+class B : public A {
+  // CHECK-NEXT: virtual void f() override {
+  virtual void f() override {
+  };
+} B;

[AaronBallman]

I don't think we should add this to the 18.x release -- we're already getting requests from users to not do that: https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20240408/566402.html

[giulianobelinassi]

I don't think we should add this to the 18.x release -- we're already getting requests from users to not do that: https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20240408/566402.html

This doesn't affect the number of spaces and it is not based on vgvassilev's patch.

The test case in question:

~/projects/llvm-project/llvm/build/bin/clang++ a.cpp -Xclang -ast-print -S -o -
template <typename T> class FinalTemplate;

This would only change the position of the override and final keywords.

[AaronBallman]

I think the 18.1 behavior is already correct?

https://godbolt.org/z/v3G7h44E1

[giulianobelinassi]

I think the 18.1 behavior is already correct?

https://godbolt.org/z/v3G7h44E1

It is not:
https://godbolt.org/z/96rWese3P

[AaronBallman]

Ah that's right, it was with definitions that we ran into the issue! Sorry for being slow today. Okay, I retract my concerns, this is a safe and targeted fix.

[AaronBallman]

I think this is reasonable, but leaving it to @erichkeane to make the final call.

[giulianobelinassi]

The branch is marked as Changes requested but no change was pointed in the code. Is everything correct?

[Sirraide]

The branch is marked as Changes requested but no change was pointed in the code. Is everything correct?

That’s just because Aaron didn’t submit another review, so github still marks it as ‘changes requested’; seeing as it has been approved, the rest is now up to the release manager.

[AaronBallman]

Sorry about leaving it in a "requests changes" state, LGTM!

[tstellar]

Hi @giulianobelinassi (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix.