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

Adding includes to cstdint #257

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Adding includes to cstdint #257

merged 2 commits into from
Jun 21, 2023

Conversation

LouieVoit
Copy link
Contributor

@LouieVoit LouieVoit commented May 15, 2023

Description

To fix #256, I've added include to the C++ cstdint in some files and changed include to stdint.h to cstdint.

In some files, there is a using namespace std hence the std::uint*_t is used but in some files it is not present. So I don't know which one is used but it compiles now. I wanted to solve the issue with as little modifications as possible in the code. But should we add the std namespace everywhere ?

How Has This Been Tested?

ls1 compiles fine with gcc 13.1.1 and clang 15.0.7. I've tried to compile with older versions of gcc and clang and it works as well.

@amartyads
Copy link
Contributor

We should actually remove the using namespace std, it shouldn't be in the production code at all. Ideally we should have std:: everywhere. Could you do that in just the files you're modifying currently, as a first step?

@FG-TUM
Copy link
Member

FG-TUM commented May 24, 2023

@LouieVoit What's the situation with this PR? Is it already reviewable?

@LouieVoit
Copy link
Contributor Author

I was about to remove the 'using namespace std' as suggested per @amartyads but then it was done in #260 . Maybe we could merge both PR ?

@FG-TUM
Copy link
Member

FG-TUM commented May 25, 2023

Your PR as it is now doesn't touch the namespaces. Let's keep things clean and separated and merge this one to master asap.

@amartyads
Copy link
Contributor

But the PR #260 doesn't add std:: in front of the uint_*s, atleast in the files Louis has changed. So it'll break the code unless either Louis or Simon update the uints.

@FG-TUM
Copy link
Member

FG-TUM commented May 25, 2023

No this will break #260 and not master. Thus, the polite way would be to add std:: in front of all special int types in this PR in the files where the headers are changed.

@amartyads amartyads added the bug Something isn't working label May 25, 2023
@LouieVoit
Copy link
Contributor Author

LouieVoit commented May 25, 2023

I've added std:: in front of all special int in the files I've modified in this PR

@LouieVoit
Copy link
Contributor Author

What is the status of this PR ? Can it be merged now ?

@FG-TUM FG-TUM merged commit 7d9f712 into ls1mardyn:master Jun 21, 2023
@LouieVoit LouieVoit deleted the fix_stdint_to_cstdint branch June 21, 2023 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting "error: ‘uint32_t’ does not name a type" with new gcc and clang versions
3 participants