This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ehsan on Apr 26 2015, 6:34 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

ehsan updated this revision to Diff 24449.Apr 26 2015, 6:34 PM
ehsan retitled this revision from to Insert override at the same line as the end of the function declaration.
ehsan updated this object.
ehsan edited the test plan for this revision. (Show Details)
ehsan added a reviewer: alexfh.
ehsan added a subscriber: Unknown Object (MLST).
alexfh edited edge metadata.May 8 2015, 3:10 AM

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.

What's the status of this patch?

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.

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.

This revision was automatically updated to reflect the committed changes.

Looks good. Thanks for the fix!