Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GCC 8.3 workaround #287

Merged
merged 8 commits into from
Dec 4, 2021
Merged

GCC 8.3 workaround #287

merged 8 commits into from
Dec 4, 2021

Conversation

nx10
Copy link
Contributor

@nx10 nx10 commented Nov 25, 2021

Compilation with GCC 8.1 - 8.3 is failing due to a compiler bug. I spent some time confirming which GCC versions are affected and it seems to be fixed in in GCC 8.4 (also fixed with GCC 9.1). (Compiler explorer: https://godbolt.org/z/beWsMznd5)

Issues referencing this problem:

This PR applies the workaround suggested in ipkn/crow#303 (comment) if one of the affected GCC versions is used. It also throws a preprocessor warning with instructions to upgrade either GCC version or C++ standard if it can not be applied.

I am not sure this workaround is in the support scope for this project, so it is fine for me if it is rejected. (It might still be helpful to stay open indefinitely for somebody who has to support GCC 8 < 8.4)

@The-EDev
Copy link
Member

Thanks a lot for the work and comprehensive PR description.

I have 2 concerns (or suggestions) however, the first being the fact that you're immediately checking the value of __GNUC__, and while it will most likely not cause issues, it might be best to check if it was defined like the case with _MSVC_LANG or _MSC_VER.
The second is that we're issuing a warning that the compilation will fail, why not just use #error instead?

When it comes to the support scope of the project, we started development with support for newer and older Boost versions, the library has a workaround for Visual Studio 13 (I believe). And we recently added C++20 support (in the sense that the program will compile out of the box). So as long as the change won't require too much expansion in the future, I don't see a reason not to integrate it.

Thanks again for your work and please let me know when the PR is ready for review. Tests are already passing and as far as I can tell there aren't any formatting errors.

@nx10
Copy link
Contributor Author

nx10 commented Nov 27, 2021

Thank you for your quick response.

I agree with and have added your suggestions.
The PR is now ready for review.

@nx10 nx10 marked this pull request as ready for review November 27, 2021 13:33
@The-EDev
Copy link
Member

The-EDev commented Nov 27, 2021

Testing it out in godbolt, I'm getting the following error when using gcc 8.1 and -std=c++14:

crow_all.h:11150:105: error: 'route' function with trailing return type has 'auto&' as its type rather than plain 'auto'

The problem seems to be that & would have to be part of the trailing return type, is the & necessary?

Everything else seems to be working fine (including the #error)

include/crow/app.h Outdated Show resolved Hide resolved
nx10 and others added 2 commits December 4, 2021 12:32
Co-authored-by: Farook Al-Sammarraie <farook@the-e-dev.com>
Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will merge once the tests are done

@The-EDev The-EDev merged commit 120e1f3 into CrowCpp:master Dec 4, 2021
@nx10 nx10 deleted the gcc8-workaround branch December 4, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants