Fix -Wextra-semi warning triggered by using a semicolon after NONCOPYABLE().
ClosedPublic
Actions

Authored by echotangoecho on May 12 2017, 2:19 PM.

Details

Reviewers

Itms

Commits

rP19626: Refactor NONCOPYABLE to remove unneeded semicolons. Remove visibility keywords…

Summary

This doesn't really change any functional aspect of the code but when one uses -Wpedantic using NONCOPYABLE(className); with the semicolon at the end currently triggers this warning. Luckily it is used everywhere in this way so changing this to a version where the semicolon isn't unneeded is easy.

Test Plan

Verify that this still disallows copying for NONCOPYABLE classes.

Verify that there are no visibility changes for any class members.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

echotangoecho created this revision.May 12 2017, 2:19 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1162/ for more details.

Harbormaster completed remote builds in B1667: Diff 1870.May 12 2017, 3:46 PM

This looks nice, I'm taking a look.

• leper edited the test plan for this revision. (Show Details)May 14 2017, 9:30 PM

A proper review of this will require someone to check that there are no visibility changes caused by this.

public:
   int foo();
NONCOPYABLE(foo);
   int bar();

Would change bar from private to public. Most code does use class Foo { NONCOPYABLE(Foo); public: ... though, so I don't expect lots of issues with this.

I went through the occurrences of the macro. The situation @leper mentions never occurs. The only thing that happens sometimes is

class Foo {
    NONCOPYABLE(Foo)
    int m_Attr1;
    int m_Attr2;
...

Since the default visibility of class attributes is private, there is no visibility change here. However, explicitly stating that those attributes are private would make our code a tad more readable. I don't have a strong opinion, what about you guys?

We could add that, but doesn't seem worth the hassle, and doesn't really make things a lot clearer.

Itms accepted this revision.May 21 2017, 3:07 PM

This revision is now accepted and ready to land.May 21 2017, 3:07 PM

Closed by commit rP19626: Refactor NONCOPYABLE to remove unneeded semicolons. Remove visibility keywords… (authored by Itms). · Explain WhyMay 21 2017, 3:11 PM

This revision was automatically updated to reflect the committed changes.