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

Allow compiling on GCC 4.8 #399

Merged
merged 5 commits into from
Apr 26, 2022
Merged

Allow compiling on GCC 4.8 #399

merged 5 commits into from
Apr 26, 2022

Conversation

The-EDev
Copy link
Member

For some reason, is_callable does not work with GCC 4.8. This is most likely a bug with g++ itself.
CallHelper, which is_callable was meant to replace seems to work fine while compiling on g++ 4.8. I've run the tests and compiled the examples, everything seems to be fine.

@dranikpg I would very much appreciate your opinion on this since you wrote is_callable and have more experience with templates than myself.

This PR is a draft because I'm not 100% sure the changes I made are sound and won't introduce bugs down the line.

Fixes #386.

Also fixed issue with ssl test string comparison (which caused random incorrect failures)
@crow-clang-format
Copy link

--- include/crow/middleware.h	(before formatting)
+++ include/crow/middleware.h	(after formatting)
@@ -248,7 +248,7 @@
           !black_magic::CallHelper<F, black_magic::S<Args...>>::value &&
             !black_magic::CallHelper<F, black_magic::S<crow::request&, Args...>>::value &&
             !black_magic::CallHelper<F, black_magic::S<crow::response&, Args...>>::value &&
-          black_magic::CallHelper<F, black_magic::S<const crow::request&, crow::response&, Args...>>::value,
+            black_magic::CallHelper<F, black_magic::S<const crow::request&, crow::response&, Args...>>::value,
           void>::type
           wrapped_handler_call(crow::request& req, crow::response& res, const F& f, Args&&... args)
         {
--- tests/ssl/ssltest.cpp	(before formatting)
+++ tests/ssl/ssltest.cpp	(after formatting)
@@ -73,7 +73,7 @@
         }
         else
         {
-            CHECK(std::string("Hello world, I'm keycrt.").substr((z*-1)) == to_test);
+            CHECK(std::string("Hello world, I'm keycrt.").substr((z * -1)) == to_test);
         }
     }
 

@dranikpg
Copy link
Member

Gcc 4.8 is so old, I even had trouble installing it 😄

CallHelper and is_callable don't differ much except for CallHelper using additional "S<>" wrappers. My bad for introducting another version of almost the same.

This seems very much like a bug to me. I'd suggest to strip is_callable out and leave only the CallHelper. They don't differ in core functionality and everything should work.

However, you've replaced checks like std::enable_if<black_magic::is_callable<F, Args...>::value>::type with negation of all previous checks. We don't have templatized handlers, so this should make no difference. Any reasons for this change I might have not noticed?

@The-EDev
Copy link
Member Author

Gcc 4.8 is so old, I even had trouble installing it

Fully agree, 9 yeas is a very long time in tech. Nevertheless IMO legacy support is still important, especially for C++.

CallHelper and is_callable don't differ much except for CallHelper using additional "S<>" wrappers. My bad for introducing another version of almost the same.

No worries, I believe the S<> wrappers worked around this specific bug.

This seems very much like a bug to me. I'd suggest to strip is_callable out and leave only the CallHelper. They don't differ in core functionality and everything should work.

Will do.

However, you've replaced checks like std::enable_if<black_magic::is_callable<F, Args...>::value>::type with negation of all previous checks. We don't have templatized handlers, so this should make no difference. Any reasons for this change I might have not noticed?

To be completely honest I wasn't sure of what I was doing, I looked at the diff in your original PR introducing is_callable and did things similar to how they were prior, I believe did run into a compilation issue when (at some point) I didn't correctly negate all the previous statements.

@The-EDev The-EDev marked this pull request as ready for review April 16, 2022 13:06
@The-EDev
Copy link
Member Author

@mrozigor @luca-schlecker Could you guys make sure there's nothing that could be a problem later on with this? I have a history of adding things that end up generating too many bugs. Cough static files Cough.

@mrozigor
Copy link
Member

Generally I think that supporting very old soft isn't a good idea ;) Even developers don't support versions earlier than 9.

@luca-schlecker
Copy link
Collaborator

I agree with @mrozigor here, GCC 4.8 is really old and I guess sticking to old tech is not only hindering progression in some cases, but can also cause a lot of trouble for little in return.

@The-EDev
Copy link
Member Author

@mrozigor @luca-schlecker I understand your point fully, on the other hand We are working with C++11, a standard older than GCC 4.0.

Since the work has already been done, would it be appropriate to merge this PR and drop GCC 4 support if the need arises later on?

@mrozigor
Copy link
Member

Code looks good to me, but I haven't run it on 4.8.

@The-EDev
Copy link
Member Author

I have, it worked fine for me

@The-EDev The-EDev merged commit 70faf20 into master Apr 26, 2022
@The-EDev The-EDev deleted the gcc48 branch April 26, 2022 02:30
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.

Compiling on GCC 4.8
4 participants