Zandronum Chat @ irc.zandronum.com
#zandronum
Get the latest version: 3.0
Source Code

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003822Doomseeker[All Projects] Cleanuppublic2020-06-08 02:472020-06-16 07:38
ReporterWubTheCaptain 
Assigned ToBlzut3 
PrioritynormalSeveritytextReproducibilityhave not tried
StatusconfirmedResolutionopen 
PlatformOSDebian GNU/LinuxOS Versionbullseye/sid
Product Version1.3.1 
Target VersionFixed in Version 
Summary0003822: [[gnu::fallthrough]] in source files (-Wunknown-attributes)
Descriptionclang++ compiler doesn't like this GNU extension when compiling, ignores it and throws a warning.
Quote from :ALEInfo
(executable check - success) clang++
(finished - exit code 0) ['/bin/mksh', '-c', '''clang++'' -S -x c++ -fsyntax-only -iquote ''/home/wub/.local/src/doomseeker/doomseeker/src/core'' 
-DINSTALL_LIBDIR=\"lib\" -DINSTALL_PREFIX=\"/usr/local\" -DQT_CORE_LIB -DQT_GUI_LIB 
-DQT_MULTIMEDIA_LIB -DQT_
NETWORK_LIB -DQT_NO_DEBUG -DQT_WIDGETS_LIB -DQT_XML_LIB -Ddoomseeker_EXPORTS -I/home/wub/.local/src/doomseeker/doomseeker/build/src/core/doomseeker_autogen/include 
-I/home/wub/.local/src/doomseeker/doomseeker/src/core/gui -I/home/wub/.local/src/doomseeker
/doomseeker/build/src/core -I/home/wub/.local/src/doomseeker/doomseeker/src/core -I/home/wub/.local/src/doomseeker/doomseeker/src/wadseeker/.. 
-isystem /usr/include/x86_64-linux-gnu/qt5 -isystem /usr/include/x86_64-linux-gnu/qt5/QtWidgets -isystem 
/usr/in
clude/x86_64-linux-gnu/qt5/QtGui -isystem /usr/include/x86_64-linux-gnu/qt5/QtCore -isystem /usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++ 
-isystem /usr/include/x86_64-linux-gnu/qt5/QtMultimedia -isystem /usr/include/x86_64-linux-gnu/qt5/QtNetwork 
-isyst
em /usr/include/x86_64-linux-gnu/qt5/QtXml -fPIC -std=gnu++11 -std=c++14 -Wall - < ''/tmp/nvimfVec8g/2/scanner.cpp''']

<<<OUTPUT STARTS>>>
<stdin>:550:7: warning: unknown attribute 'fallthrough' ignored [-Wunknown-attributes]
                                [[gnu::fallthrough]];
                                  ^
1 warning generated.
<<<OUTPUT ENDS>>>
Steps To Reproduce
$ grep -ir "fallthrough" src/
src/wadseeker/zip/unzip.cpp:                            [[gnu::fallthrough]];
src/core/joincommandlinebuilder.cpp:            [[gnu::fallthrough]];
src/core/refresher/refresher.cpp:                       [[gnu::fallthrough]];
src/core/scanner.cpp:                           [[gnu::fallthrough]];
Additional Informationclang 9.0; see note 0003822:0021425
Attached Files

- Relationships

-  Notes
User avatar (0021417)
WubTheCaptain (developer)
2020-06-08 02:50
edited on: 2020-06-08 07:53

What should be done about this?
https://en.cppreference.com/w/cpp/language/attributes/fallthrough [^]
https://clang.llvm.org/docs/AttributeReference.html#fallthrough [^]
I don't know why clang didn't like this vendor extension, maybe because ALE in NVim defaults to -std=c++14 (without GNU extensions).
Anyway, [[fallthrough]] is valid since C++17. That's a far standards target at the moment (Doomseeker is targeting C++11). A comment /* fallthrough */ would be the most portable option, but I don't mind going with the C++17 [[fallthrough]] feature either.

User avatar (0021418)
WubTheCaptain (developer)
2020-06-08 03:37

https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/ [^]
User avatar (0021419)
WubTheCaptain (developer)
2020-06-08 03:40
edited on: 2020-06-08 03:40

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0068r0.pdf [^]
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0188r1.pdf [^]

User avatar (0021420)
Blzut3 (administrator)
2020-06-08 04:20

Surprised Clang doesn't support the gnu version, but yes the proper course of action is to change this to the C++17 version now that it's standardized. Since unknown attributes are not an error, there's no issues with using this C++17 feature without requiring C++17.
User avatar (0021425)
WubTheCaptain (developer)
2020-06-08 06:32
edited on: 2020-06-08 06:40

Quote from Blzut3
Surprised Clang doesn't support the gnu version,

I just found out it's not supported by clang 9.0, but it's supported by clang 10.0. My machine in OP was using clang 9.0 (Debian bullseye/sid).
https://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html [^]
https://releases.llvm.org/10.0.0/tools/clang/docs/AttributeReference.html [^]
Quote from Blzut3
but yes the proper course of action is to change this to the C++17 version now that it's standardized. Since unknown attributes are not an error, there's no issues with using this C++17 feature without requiring C++17

I think you're correct and incorrect.
Though I didn't test it, Intel's C++ compiler will treat unknown attributes as errors (I've read). Affects VSCode 2017 or 2019? (Fixed since April 2019?)
N3337 draft for C++11 (page 168, dcl.attr.grammar, § 7.6.1) says:
Quote from N3337 & N4140
For an attribute-token not specified in this International Standard, the behavior is implementation-defined.

No word if it's not recognized, so compilers can either throw either an error, a warning, or just ignore it. Or do whatever. This remains so in N4140 (C++14 standard draft).
In N4659 draft for C++17, this was changed (page 195, dcl.attr.grammar, § 10.6.1):
Quote from N4659
For an attribute-token (including an attribute-scoped-token) not specified in this International Standard, the behavior is implementation-defined. Any attribute-token that is not recognized by the implementation is ignored. [Note: Each implementation should choose a distinctive name for the attribute-namespace in anattribute-scoped-token. — end note]

So the unknown attributes are not an error only since C++17.
I think we'd have to change CMake's CXX_STANDARD to target C++17 to ensure the behavior is correct in every case (with a standards compliant C++ compiler) to use [[fallthrough]]. Pedantically speaking.
Or I could use /* fallthrough */ for the time being and leave CXX_STANDARD be C++11.

User avatar (0021426)
WubTheCaptain (developer)
2020-06-08 07:02

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0283r2.html [^]
and that MSVC (VSCode) issue:https://developercommunity.visualstudio.com/content/problem/315417/error-c2429-on-fallthrough.html [^]
User avatar (0021427)
WubTheCaptain (developer)
2020-06-08 07:34
edited on: 2020-06-08 07:43

I believe __attribute__((fallthrough)) was a GNU extension too (supported for C and C++98 since GCC 7, while [[gnu::fallback]] is a GNU extension for C++11 and C++14), so it's not proposed here.
I'm not sure when exactly [[gnu::fallback]] was introduced, but I'd have to guess GCC 7.

Relying on __cplusplus definition with the preprocessor was not reliable on MSVC:https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ [^]
Nor on GCC (before GCC 4.7):https://gcc.gnu.org/bugzilla/show_bug.cgi?id=1773 [^]
Plus, it'd be ugly to hack standards detection that way (and may cause problems in the future). So I'm only in for supporting one decision: Either the /* fallthrough */ comment style everywhere, or compiling against C++17 standard.

So anyway, this affects (at least) GCC versions < 7, clang versions < 10, and Visual Studio 2019 Version < 16.1.

User avatar (0021428)
WubTheCaptain (developer)
2020-06-08 07:47
edited on: 2020-06-08 07:50

#if __has_cpp_attribute(fallthrough)
[[fallthrough]]
#else
/* fallthrough */
#endif

🤪

I'd prefer just one way of doing it, not preprocessor heck. (Forget what I said about __cplusplus version checking, that's not reliable for feature compliance anyway.)

And that hack doesn't check if __has_cpp_attribute is defined, so no thanks for more hacks.
https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations#testing-for-the-presence-of-an-attribute-__has_cpp_attribute [^]

User avatar (0021433)
Pol M (developer)
2020-06-08 13:03
edited on: 2020-06-08 13:08

Quote from Wub

Though I didn't test it, Intel's C++ compiler will treat unknown attributes as errors (I've read). Affects VSCode 2017 or 2019? (Fixed since April 2019?)

(standarized) attributes are intended to express intent both to the programmers and to the compiler. They are designed very explicitly to allow code with and without them to compile successfully and with the same behaviour. The compilers were technically on the wrong here. That said, intel's compiler and Visual Studio are not supported by the project, so we should only worry about gcc, mingw-w64 (which is basically gcc) and clang as it is pretty much gcc compatible.
Quote from Wub

I think we'd have to change CMake's CXX_STANDARD to target C++17 to ensure the behavior is correct in every case (with a standards compliant C++ compiler) to use [[fallthrough]]. Pedantically speaking.

While not wrong, pretty much the course of action of any vendor is going to be to not error regardless the standard you use.

If I recall correctly, I introduced this tag. We considered whether to break the laws a bit and just use [[fallthrough]], use a comment (which I opposed as I wanted something more formal) or use [[gnu::fallthrough]]. At the end, this last won as it was supported by gcc and mingw-w64 and that was the priority at the moment. I knew that would throw a warning in clang a that time, but since attributes doesn't change the code I considered it a small inconvenience in exchange for being able to compile with all the warnings enabled and being able to not get spammed with fallthrough warnings. I don't think it is a good idea to open the door to just a few C++>11 features as that just leads to extra warnings in compilers. That said, if we decide to move to C++17, I'll be happy to change them all to [[fallthrough]]

User avatar (0021438)
WubTheCaptain (developer)
2020-06-08 14:01
edited on: 2020-06-08 14:13

I'll leave the decision to you, Pol M. But I'll mention my preference and encouragement to be as portable and standards compliant as possible.

User avatar (0021441)
WubTheCaptain (developer)
2020-06-08 14:15

I will also mention the OpenBSD style(9) is to use a /* FALLTHROUGH */ comment (for C).
https://man.openbsd.org/OpenBSD-6.6/style.9 [^]
Not entirely accurate because our code is C++, but compilers like GCC will recognize those comments to suppress the warnings.
User avatar (0021444)
WubTheCaptain (developer)
2020-06-08 14:41
edited on: 2020-06-08 14:45

0003806:0021443:
Quote from Blzut3
Per the rule that we support Ubuntu releases until their EOL, we currently have to support GCC 5 which is only C++14.

The GCC version may be of importance here.

User avatar (0021448)
Blzut3 (administrator)
2020-06-08 21:17

Hmm, I don't recall the discussion on gnu::fallthrough vs fallthrough and it was added recently enough that I'm not entirely sure why we would have gone with the former. I'm fine with moving to the standard attribute and just letting GCC 5 throw a warning (or turn off that warning when compiling with older versions of GCC).
User avatar (0021464)
Pol M (developer)
2020-06-15 00:25

Oh, if Blzut is ok with introducing a small C++17 thing, I won't block the way then.

Issue Community Support
Only registered users can voice their support. Click here to register, or here to log in.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2020-06-08 02:47 WubTheCaptain New Issue
2020-06-08 02:50 WubTheCaptain Note Added: 0021417
2020-06-08 02:50 WubTheCaptain Assigned To => WubTheCaptain
2020-06-08 02:50 WubTheCaptain Status new => acknowledged
2020-06-08 03:37 WubTheCaptain Note Added: 0021418
2020-06-08 03:40 WubTheCaptain Note Added: 0021419
2020-06-08 03:40 WubTheCaptain Note Edited: 0021419 View Revisions
2020-06-08 04:20 Blzut3 Note Added: 0021420
2020-06-08 06:32 WubTheCaptain Note Added: 0021425
2020-06-08 06:32 WubTheCaptain Status acknowledged => confirmed
2020-06-08 06:35 WubTheCaptain OS => Debian GNU/Linux
2020-06-08 06:35 WubTheCaptain OS Version => bullseye/sid
2020-06-08 06:35 WubTheCaptain Additional Information Updated View Revisions
2020-06-08 06:38 WubTheCaptain Note Edited: 0021425 View Revisions
2020-06-08 06:40 WubTheCaptain Note Edited: 0021425 View Revisions
2020-06-08 07:02 WubTheCaptain Note Added: 0021426
2020-06-08 07:34 WubTheCaptain Note Added: 0021427
2020-06-08 07:35 WubTheCaptain Note Edited: 0021427 View Revisions
2020-06-08 07:36 WubTheCaptain Note Edited: 0021427 View Revisions
2020-06-08 07:40 WubTheCaptain Note Edited: 0021427 View Revisions
2020-06-08 07:42 WubTheCaptain Note Edited: 0021427 View Revisions
2020-06-08 07:43 WubTheCaptain Note Edited: 0021427 View Revisions
2020-06-08 07:47 WubTheCaptain Note Added: 0021428
2020-06-08 07:49 WubTheCaptain Note Edited: 0021428 View Revisions
2020-06-08 07:50 WubTheCaptain Note Edited: 0021428 View Revisions
2020-06-08 07:53 WubTheCaptain Note Edited: 0021417 View Revisions
2020-06-08 13:03 Pol M Note Added: 0021433
2020-06-08 13:08 Pol M Note Edited: 0021433 View Revisions
2020-06-08 14:01 WubTheCaptain Note Added: 0021438
2020-06-08 14:01 WubTheCaptain Assigned To WubTheCaptain => Pol M
2020-06-08 14:13 WubTheCaptain Note Edited: 0021438 View Revisions
2020-06-08 14:13 WubTheCaptain Note Edited: 0021438 View Revisions
2020-06-08 14:15 WubTheCaptain Note Added: 0021441
2020-06-08 14:41 WubTheCaptain Note Added: 0021444
2020-06-08 14:45 WubTheCaptain Note Edited: 0021444 View Revisions
2020-06-08 21:17 Blzut3 Note Added: 0021448
2020-06-15 00:25 Pol M Note Added: 0021464
2020-06-15 00:26 Pol M Assigned To Pol M =>
2020-06-15 00:27 Pol M Status confirmed => feedback
2020-06-16 07:36 WubTheCaptain Status feedback => confirmed
2020-06-16 07:38 WubTheCaptain Assigned To => Blzut3






Questions or other issues? Contact Us.

Links


Copyright © 2000 - 2020 MantisBT Team
Powered by Mantis Bugtracker