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

recast 64-bit size_t to 32-bit int to remove warnings #106

Merged
merged 4 commits into from
Apr 2, 2021

Conversation

sfinktah
Copy link
Contributor

There are many things defined as int accepting size_t arguments.

You could change those things to accept size_t arguments, or you can silence the warnings.

I have provided a solution for the latter, since it's not my place to change your structures.

With all my PRs and the two response callbacks that return void removed, this project will now compile without errors or warnings.

@The-EDev The-EDev requested a review from mrozigor February 11, 2021 22:52
@mrozigor
Copy link
Member

Sorry for delayed response. Are those error MSVC specific? I tried to build example with all error/warning flags and nothing happened. Also I don't think that workarounds are good solution.

@sfinktah
Copy link
Contributor Author

@mrozigor There are at least 6 instances of assigning variables defined as size_t to containers defined as int.

The only way that could possibly not cause warnings is if you were compiling on a 32-bit platform where size_t is the same size as int. Try adding -Wconversion, it's a common misconception that -Wall activates all warnings.

@mrozigor
Copy link
Member

Mea Culpa, I've forgotten about this. It looks like there are more places with conversion problems. I'll try to run compilation with error/warning flags and fix those. For now - can you change casts from (...) to static_cast? Thanks.

@sfinktah
Copy link
Contributor Author

Mea Culpa, I've forgotten about this. It looks like there are more places with conversion problems. I'll try to run compilation with error/warning flags and fix those. For now - can you change casts from (...) to static_cast? Thanks.

Sure... fortunately I use ReSharper so I should be able to automate that process.

@sfinktah
Copy link
Contributor Author

@mrozigor Done. It found a lot more than the ones I added, but it only took 60 seconds for it to change them all to static_cast.

@sfinktah
Copy link
Contributor Author

@mrozigor wrong place to ask, but never the less -- in actually trying to use crow (after all this work, I felt I should), I noted that it doesn't return control (i.e. doesn't thread itself). that's totally fine, i can launch it in it's own thread -- i was just wondering if there was a recommended method, i.e. boost, std::thread, std::async or whatnot.

And more importantly -- how to terminate the thread (and/or crow) properly afterwards.

@mrozigor
Copy link
Member

mrozigor commented Mar 6, 2021

Again - sorry for late response. With this PR we are still waiting for Travis CI team to respond. Probably will have to setup project on my private Jenkins server and connect it with repo.

Weren't you able to stop() server?

@The-EDev The-EDev closed this Apr 1, 2021
@The-EDev The-EDev reopened this Apr 1, 2021
@The-EDev
Copy link
Member

The-EDev commented Apr 1, 2021

@sfinktah could you please merge master into this PR so CI could run?

@The-EDev The-EDev merged commit 5ab2607 into CrowCpp:master Apr 2, 2021
@sfinktah
Copy link
Contributor Author

sfinktah commented May 9, 2021

@The-EDev sorry, I have been quite busy.

Weren't you able to stop() server?

I haven't decided the best to launch it in another thread – part of which is the question of how to keep a handle to call the stop() on. Of course a global would work, but it seems rather yuck.

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.

3 participants