Insert override at the same line as the end of the function declaration

[• ehsan]

Summary

The existing check converts the code pattern below:

void f()
{
}

to:

void f()
override {
}

which is fairly sub-optimal. This patch fixes this by inserting the
override keyword on the same line as the function declaration if
possible, so that we instead get:

void f() override
{
}

We do this by looking for the last token before the start of the body
and inserting the override keyword at the end of its location. Note
that we handle const, volatile and ref-qualifiers correctly.

[alexfh]

Sorry for the long delay. I was on vacation and now digging out stuff of my inbox.

Thanks for the patch! I've got a few comments regarding the implementation though.

clang-tidy/misc/UseOverrideCheck.cpp

158 ↗

(On Diff #24449)

Can you just find the last non-comment token before the opening brace?

159 ↗

(On Diff #24449)

Please use the braces for all branches, if they are used for at least one. Also, run clang-format on the new code.

[alexfh]

What's the status of this patch?

[• ehsan]

I should land this, but let's wait to see if we want http://reviews.llvm.org/D9285 or not, since this patch is built on top of it. If not, I can rebase it on top of master as well.

[alexfh]

In D9286#224691, @ehsan wrote:

I should land this, but let's wait to see if we want http://reviews.llvm.org/D9285 or not, since this patch is built on top of it. If not, I can rebase it on top of master as well.

We don't want D9285, so this patch should be rebased on top of master. Please also address the inline comments.

[alexfh]

Looks good. Thanks for the fix!

clang-tools-extra/trunk/clang-tidy/misc/UseOverrideCheck.cpp

if (Method->hasAttrs()) {
           if (!InsertLoc.isValid() ||
               Sources.isBeforeInTranslationUnit(Loc, InsertLoc))
             InsertLoc = Loc;
         }
       }
     }
 
     if (InsertLoc.isInvalid() && Method->doesThisDeclarationHaveABody() &&
-        Method->getBody() && !Method->isDefaulted())
-      InsertLoc = Method->getBody()->getLocStart();
+        Method->getBody() && !Method->isDefaulted()) {
+      // For methods with inline definition, add the override keyword at the
+      // end of the declaration of the function, but prefer to put it on the
+      // same line as the declaration if the beginning brace for the start of
+      // the body falls on the next line.
+      Token LastNonCommentToken;
+      for (Token T : Tokens) {
+        if (!T.is(tok::comment)) {
+          LastNonCommentToken = T;
+        }
+      }
+      InsertLoc = LastNonCommentToken.getEndLoc();
+      ReplacementText = " override";
+    }
 
     if (!InsertLoc.isValid()) {
       // For declarations marked with "= 0" or "= [default|delete]", the end
       // location will point until after those markings. Therefore, the override
       // keyword shouldn't be inserted at the end, but before the '='.
       if (Tokens.size() > 2 && (GetText(Tokens.back(), Sources) == "0" ||
                                 Tokens.back().is(tok::kw_default) ||
                                 Tokens.back().is(tok::kw_delete)) &&

clang-tools-extra/trunk/test/clang-tidy/misc-use-override.cpp

struct Base {
   virtual void j() const;
   virtual MustUseResultObject k();
   virtual bool l() MUST_USE_RESULT UNUSED;
   virtual bool n() MUST_USE_RESULT UNUSED;
 
   virtual void m();
   virtual void m2();
   virtual void o() __attribute__((unused));
+
+  virtual void r() &;
+  virtual void rr() &&;
+
+  virtual void cv() const volatile;
+  virtual void cv2() const volatile;
 };
 
 struct SimpleCases : public Base {
 public:
   virtual ~SimpleCases();
   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual'
   // CHECK-FIXES: {{^}}  ~SimpleCases() override;
 
public:
   void a() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this
   // CHECK-FIXES: {{^}}  void a() override {}
 
   void b() override {}
   // CHECK-MESSAGES-NOT: warning:
   // CHECK-FIXES: {{^}}  void b() override {}
 
-  virtual void c() {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
-  // CHECK-FIXES: {{^}}  void c() override {}
+  virtual void c()
+  {}
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void c() override
 
   virtual void d() override {}
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant
   // CHECK-FIXES: {{^}}  void d() override {}
 
-  virtual void j() const {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
-  // CHECK-FIXES: {{^}}  void j() const override {}
+  virtual void j() const
+  {}
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void j() const override
 
   virtual MustUseResultObject k() {}  // Has an implicit attribute.
   // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: prefer using
   // CHECK-FIXES: {{^}}  MustUseResultObject k() override {}
 
   virtual bool l() MUST_USE_RESULT UNUSED {}
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
   // CHECK-FIXES: {{^}}  bool l() override MUST_USE_RESULT UNUSED {}
+
+  virtual void r() &
+  {}
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void r() & override
+
+  virtual void rr() &&
+  {}
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void rr() && override
+
+  virtual void cv() const volatile
+  {}
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void cv() const volatile override
+
+  virtual void cv2() const volatile // some comment
+  {}
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void cv2() const volatile override // some comment
 };
 
 struct Macros : public Base {
   // Tests for 'virtual' and 'override' being defined through macros. Basically
   // give up for now.
   NOT_VIRTUAL void a() NOT_OVERRIDE;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: annotate this
   // CHECK-FIXES: {{^}}  NOT_VIRTUAL void a() override NOT_OVERRIDE;