Page MenuHomePhabricator

Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse
ClosedPublic

Authored by Rob on Mar 23 2016, 8:02 AM.

Details

Summary

This patch is to address 2 problems I found with Clang-tidy:modernize-use-override.

1: missing spaces on pure function decls.

Orig:
void pure() const=0
Problem:
void pure() constoverride =0
Fixed:
void pure() const override =0

2: This is ms-extension specific, but possibly applies to other attribute types. The override is placed before the attribute which doesn’t work well with declspec as this attribute can be inherited or placed before the method identifier.

Orig:
class __declspec(dllexport) X : public Y

{
void p();
};

Problem:
class override __declspec(dllexport) class X : public Y

{
void p();
};

Fixed:
class __declspec(dllexport) class X : public Y

{
void p() override;
};

Diff Detail

Repository
rL LLVM

Event Timeline

Rob updated this revision to Diff 51417.Mar 23 2016, 8:02 AM
Rob retitled this revision from to Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse.
Rob updated this object.
Rob added reviewers: alexfh, aaron.ballman.
Rob added a subscriber: cfe-commits.
alexfh edited edge metadata.Mar 23 2016, 9:34 AM

Thank you for the patch!

Looks good in general. A few nits.

clang-tidy/modernize/UseOverrideCheck.cpp
125 ↗(On Diff #51417)

nit: no need for the parentheses around the function call.

136 ↗(On Diff #51417)

Please clang-format this change.

test/clang-tidy/modernize-use-override-ms.cpp
1 ↗(On Diff #51417)

Does this test pass on linux? If no additional compiler arguments needed and it just works, maybe just merge this test code to the main test file?

Rob updated this revision to Diff 51904.Mar 29 2016, 7:23 AM
Rob edited edge metadata.

Update to address review.
Clang-format applied and nit fixed.

aaron.ballman added inline comments.Mar 29 2016, 8:28 AM
test/clang-tidy/modernize-use-override-ms.cpp
1 ↗(On Diff #51904)

I *think* this run line may require -fms-extensions in order to compile under non-MSVC-built versions of clang because of the __declspec.

Please summarize the improvements in docs/ReleaseNotes.rst in the clang tidy section.

Rob updated this revision to Diff 52227.Mar 31 2016, 9:33 AM

I tried

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp -- -- -fms-extensions

and

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp

on OSX and nethier work as expected.

So instead I have changed the test to use visibility attributes when _MSC_VER is not defined.

Also updated the release notes.

In D18396#388287, @Rob wrote:

I tried

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp -- -- -fms-extensions

It also needs -std=c++11, which needs to be specified, when you specify any options manually.

and

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp

on OSX and nethier work as expected.

So instead I have changed the test to use visibility attributes when _MSC_VER is not defined.

... which means that the code is not tested on non-windows platforms, though it could be.

Also updated the release notes.

Thanks!

test/clang-tidy/modernize-use-override-ms.cpp
1 ↗(On Diff #51904)

I've just tried this on Linux and it actually needs -fms-extensions (actually, you'll need to append -- -- -std=c++11 -fms-extensions).

In D18396#388287, @Rob wrote:

I tried

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp -- -- -fms-extensions

It also needs -std=c++11, since you specify options manually and "opt out" of the defaults (which are currently -std=c++11).

and

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp

on OSX and nethier work as expected.

So instead I have changed the test to use visibility attributes when _MSC_VER is not defined.

... which means that the code is not tested on non-windows platforms, though it could be.

Also updated the release notes.

Thanks!

docs/ReleaseNotes.rst
73 ↗(On Diff #52227)

nit: Please enclose __declspec in double backquotes.

alexfh requested changes to this revision.Apr 1 2016, 6:04 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 1 2016, 6:04 PM
Rob updated this revision to Diff 52539.EditedApr 4 2016, 3:40 AM
Rob edited edge metadata.

adding -std=c++11 and thought it worked as expected so uploaded diff4.

In fact it is just ignoring the attributes, so its not actually testing the problem.

I propose going back to revision 3 where one of the two tests did actually fail on osx, without my fix. The test where the attribute on the class decl that is inherited by the function decl is not reproducable using attribute(visibility)

warning: __declspec attribute 'dllexport'

is not supported [-Wignored-attributes]
alexfh accepted this revision.Apr 4 2016, 4:29 AM
alexfh edited edge metadata.

Looks good with one nit. Thank you for the patch!

Please tell, if you need someone to commit the patch for you.

docs/ReleaseNotes.rst
73 ↗(On Diff #52539)

Please enclose inline code snippets (__declspec) in double backquotes.

This revision is now accepted and ready to land.Apr 4 2016, 4:29 AM
alexfh added inline comments.Apr 4 2016, 4:30 AM
test/clang-tidy/modernize-use-override-ms.cpp
2 ↗(On Diff #52539)

An old comment sneaked in somehow. Please ignore.

Rob updated this revision to Diff 52543.Apr 4 2016, 5:04 AM
Rob edited edge metadata.

Adding back quotes in the docs

Rob added a comment.Apr 4 2016, 5:09 AM

ok, I'm not sure if its worth rolling back the change to modernize-use-override-ms.cpp, see ammended comments above.

Apart from this it should be good to go.

I think I am going to need someone to submit the patch for me, I can't use svn due to 'error 400' so I've been using the git mirror.

test/clang-tidy/modernize-use-override-ms.cpp
1 ↗(On Diff #51417)

That's a good question. I have been running it with the additional -fms-extensions argument, but on windows I find it works with or without that option. I havent tested it on linux. I could maybe rummage up a virtual machine and give it a go.

alexfh added a comment.Apr 4 2016, 7:10 AM
In D18396#391031, @Rob wrote:

ok, I'm not sure if its worth rolling back the change to modernize-use-override-ms.cpp, see ammended comments above.

No, the test is fine as it is.

Apart from this it should be good to go.

I think I am going to need someone to submit the patch for me, I can't use svn due to 'error 400' so I've been using the git mirror.

If you don't know for sure, you don't have the commit access. I'll commit the patch for you.

test/clang-tidy/modernize-use-override-ms.cpp
2 ↗(On Diff #52543)

Everything's fine with the test as it is now: both -fms-extensions and -std=c++11 are needed on non-windows platforms.

alexfh added a comment.Apr 4 2016, 7:12 AM

The patch doesn't apply cleanly. Please rebase it on top of HEAD.

This revision was automatically updated to reflect the committed changes.
alexfh added a comment.Apr 4 2016, 7:41 AM

The patch doesn't apply cleanly. Please rebase it on top of HEAD.

Actually, the conflict was only in the ReleaseNotes.rst file, which I had to change anyway to fix the formatting around the place you changed. The patch is committed as http://reviews.llvm.org/rL265298. Thank you!

Rob added a comment.Apr 4 2016, 9:05 AM

Thanks :)