Paths

Table of Contentst

Support [[fallthrough]].
ClosedPublic
Actions

Authored by • leper on Jul 12 2017, 11:41 PM.

Details

Reviewers

echotangoecho
wraitii
Itms
elexis

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Owners Package	(Owns No Changed Paths)

Commits

rP20095: Add FALLTHROUGH, which in the best case is just [[fallthrough]].

Summary

Note that this does not handle MSVC which supports this since VS2017 with some compiler flag.
It also assumes that a compiler that supports the attribute also supports __has_cpp_attribute,
but this might not be valid everywhere (eg MSVC does not support the latter according to documentation).

(I decided to define [[fallthrough]] on compilers that do not support it, so future updates need to do less work, and so we can actually use something that is standard (even if not the one we support. This might not be best practice (since it isn't obvious that it is a define and thus handled (via includes in precompiled.h) by older compilers), but we do have something similar for C99's __func__ and I like the eased upgrade path.)

Test Plan

Test this on old and new compilers, and other platforms and whatnot.

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

• leper created this revision.Jul 12 2017, 11:41 PM

Owners added a subscriber: Restricted Owners Package.Jul 12 2017, 11:41 PM

Build has FAILED

Updating workspaces.
Build (release)...
In file included from ../../../source/lib/precompiled.h:65:0,
                 from ../../../source/mocks/mocks_real.cpp:26:
../../../source/lib/code_annotation.h:398:10: error: macro names must be identifiers
 # define [[fallthrough]]
          ^
In file included from ../../../source/lib/precompiled.h:65:0,
                 from ../../../source/pch/network/precompiled.h:19:
../../../source/lib/code_annotation.h:398:10: error: macro names must be identifiers
 # define [[fallthrough]]
          ^
make[1]: *** [obj/mocks_real_Release/mocks_real.o] Error 1
make: *** [mocks_real] Error 2
make: *** Waiting for unfinished jobs....
make[1]: *** [obj/network_Release/precompiled.h.gch] Error 1
make: *** [network] Error 2

Link to build: http://jw:8080/job/phabricator/1737/
See console output for more information: http://jw:8080/job/phabricator/1737/console

Harbormaster failed remote builds in B2595: Diff 2899!Jul 13 2017, 1:21 AM

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/310/ for more details.

In D740#29151, @Vulcan wrote:

Build has FAILED

I guess that means going with FALLTHROUGH or using comments that pass, meh.

Use FALLTHROUGH. I somewhat dislike this, since we'll have to replace it once we don't need to support old compilers. Opinions on whether we should do this (and thus avoid future changes to the warning option) or go with comments that might not help based on that warning option?

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!
Checking XML files...

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

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/315/ for more details.

Harbormaster completed remote builds in B2600: Diff 2904.Jul 13 2017, 4:32 AM

Not adding code that makes non-recent compilers (like the one used by Vulcan / Jenkins) throw compiler warnings helps with not getting bug reports.
Marking fall throughs in the code and potentially adding a hint to the code guidelines will make it easier to transition while not adding any disadvantage afaics.
(Looks correct, so a reviewer could do some quick grep to check for completeness.)

source/lib/sysdep/os/unix/unix.cpp
1 ↗	(On Diff #2904)	(C)

In D740#29215, @elexis wrote:

Not adding code that makes non-recent compilers (like the one used by Vulcan / Jenkins) throw compiler warnings helps with not getting bug reports.

Marking fall throughs in the code and potentially adding a hint to the code guidelines will make it easier to transition while not adding any disadvantage afaics.

This does fix warnings thrown by recent GCC (7) (and some recent Clang (though I'm not sure if it is enabled with our warning options, didn't bother to check)) with -Wimplicit-fallthrough (which defaults to 3), and then replacing a few places with comments that also silence the warning.

(Looks correct, so a reviewer could do some quick grep to check for completeness.)

Best would be using recent GCC and manually adding -Wimplicit-fallthrough=5 and building.

There are still some fallthrough warnings in AtlasObjectXML.cpp, is that expected?

source/lib/code_annotation.h
391 ↗	(On Diff #2904)	Is `__has_cpp_attribute` supported everywhere? I think it is not standard C++: http://en.cppreference.com/w/cpp/experimental/feature_test (and it doesn't appear in the latest standard draft I have).

In D740#29762, @echotangoecho wrote:

There are still some fallthrough warnings in AtlasObjectXML.cpp, is that expected?

Nice catch, I guess I missed those since wxWidget headers are quite noisy with some redefinition warnings.

source/lib/code_annotation.h
391 ↗	(On Diff #2904)	It is included in [1], and while not yet included in any standard or draft, it is supported by 2/3s of our supported compilers (which is why there is the `\|\| ...` part there), and given MSs recent work on actually implementing the C++ standard I somewhat suspect that this is likely to show up in some future version. That said it is defined in compiler.h below, which makes this work in our code (it's pulled in by precompiled.h). [1] https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations#recs.hasattr

In D740#29795, @leper wrote:

In D740#29762, @echotangoecho wrote:

There are still some fallthrough warnings in AtlasObjectXML.cpp, is that expected?

Nice catch, I guess I missed those since wxWidget headers are quite noisy with some redefinition warnings.

Given that that is part of the Atlas shared lib, which doesn't include anything from the rest of source/ the only options for that are ignoring it, adding comments that silence the warning, or adding the same boiler-plate and using that. Also it seems that thing is most likely just used by the Actor Editor, which I guess isn't something I care about enough to fix this, if someone else cares enough they should look at the rest of the file (and possibly consider using ICU for the conversion code).

Then it's ok, but could maybe add the comments for that in the future.

This revision is now accepted and ready to land.Aug 17 2017, 5:47 PM

Tested with

uname -a
Linux debian 4.12.0-1-amd64 #1 SMP Debian 4.12.6-1 (2017-08-12) x86_64 GNU/Linux

uname -r
4.12.0-1-amd64

gcc --version
gcc (Debian 7.1.0-13) 7.1.0

Compilation is ok.
I have warnings in ../../../source/tools/atlas/AtlasObject/AtlasObjectXML.cpp (as described, discussed in previous comments).

source/graphics/CinemaManager.cpp
261 ↗	(On Diff #2904)	(All that has been removed in rP19980)

Closed by commit rP20095: Add FALLTHROUGH, which in the best case is just [[fallthrough]]. (authored by • leper). · Explain WhySep 1 2017, 10:05 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

ps/

trunk/

source/

gui/

CInput.cpp

2 lines

GUItext.cpp

8 lines

lib/

code_annotation.h

14 lines

sysdep/

compiler.h

6 lines

os/

unix/

unix.cpp

6 lines

ps/

Preprocessor.cpp

3 lines

renderer/

TexturedLineRData.cpp

2 lines

simulation2/

components/

CCmpUnitMotion.cpp

2 lines

Diff 3461

View Options

ps/trunk/source/gui/CInput.cpp

View Options

ps/trunk/source/gui/GUItext.cpp

View Options

ps/trunk/source/lib/code_annotation.h

View Options

ps/trunk/source/lib/sysdep/compiler.h

View Options

ps/trunk/source/lib/sysdep/os/unix/unix.cpp

View Options

ps/trunk/source/ps/Preprocessor.cpp

View Options

ps/trunk/source/renderer/TexturedLineRData.cpp

View Options

ps/trunk/source/simulation2/components/CCmpUnitMotion.cpp

Support [[fallthrough]].ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 3461

ps/trunk/source/gui/CInput.cpp

ps/trunk/source/gui/GUItext.cpp

ps/trunk/source/lib/code_annotation.h

ps/trunk/source/lib/sysdep/compiler.h

ps/trunk/source/lib/sysdep/os/unix/unix.cpp

ps/trunk/source/ps/Preprocessor.cpp

ps/trunk/source/renderer/TexturedLineRData.cpp

ps/trunk/source/simulation2/components/CCmpUnitMotion.cpp

Support [[fallthrough]].
ClosedPublic
Actions

Revision Contents
Changeset List