Skip to content

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

5 participants