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

[WIP] pedantic and many more flags for GNU compiler #662

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

psychocrypt
Copy link
Collaborator

@psychocrypt psychocrypt commented Dec 22, 2017

Activate many compiler checks (pedantic, ...) for the GNU compiler
to avoid side effects, undefined behaviors and increase the code
quality.

  • change timer variables from uin64_t to int64_t
  • fix conversion issues form unsigned to signed
  • remove usage of non C++ variable length arrays e.g int x[N] where N is a runtime value

Tests

  • rebase against @fireice-uk timer zone shift bug fix
  • test windows (compile and runtime)
  • test clang (compiler)
  • test NVIDIA and AMD runtime

@fireice-uk Do to the timer changes from uint64_t to int64_t this PR can conflict with you upcoming changes. This PR should be rebased against you changes. Maybe we should merge that PR not before the Xmas release to have a longer time to test the changes within the next dev branch.

@fireice-uk
Copy link
Owner

remove usage of non C++ variable length arrays e.g int x[N] where N is a runtime value

Don't think that's the case anywhere in the code, otherwise it wouldn't compile on MSVC

@psychocrypt
Copy link
Collaborator Author

psychocrypt commented Dec 22, 2017 via email

@psychocrypt psychocrypt force-pushed the topic-pedanticCompilerFlags branch 2 times, most recently from 8ab8e29 to c194af0 Compare December 25, 2017 21:15
@XhmikosR
Copy link

XhmikosR commented Dec 27, 2017

This is very nice to see!

Have you guys thought about Travis CI for nix and AppVeyor for Windows CI? Scratch that, you already have this; I just didn't see any badges in README.md.

Also, how about running Coverity which is a static code analyzer on CI?

@fireice-uk
Copy link
Owner

@psychocrypt what's the rationale behind having signed time-stamps?

@psychocrypt
Copy link
Collaborator Author

@fireice-uk The reason is that int64_t is the default type used in the stl. This make sense else it would not be possible to define a negative offset i time.

@fireice-uk
Copy link
Owner

fireice-uk commented Dec 28, 2017

Timestamp is an absolute value and not an offset, confusion on that part is why I always use unsigned type for that. If those are the only grounds then I'm inclined to veto that part.

@psychocrypt
Copy link
Collaborator Author

psychocrypt commented Dec 28, 2017 via email

@psychocrypt
Copy link
Collaborator Author

psychocrypt commented Dec 28, 2017 via email

@fireice-uk
Copy link
Owner

Given that the value is never going to be negative it only serves to confuse. I prefer the way we are doing it now.

@psychocrypt
Copy link
Collaborator Author

psychocrypt commented Dec 28, 2017 via email

@psychocrypt psychocrypt force-pushed the topic-pedanticCompilerFlags branch from c194af0 to 90a6236 Compare December 28, 2017 20:09
Activate many compiler checks (pedantic, ...) for the GNU compiler
to avoid side effects, undefined behaviors and increase the code
quallity.
@psychocrypt psychocrypt force-pushed the topic-pedanticCompilerFlags branch 2 times, most recently from 958f12c to 686baf4 Compare December 28, 2017 20:41
- change timer variables from `uin64_t` to `int64_t`
- fix conversion issues form unsigned to signed
- remove usage of non C++ variable length arrays e.g `int x[N]` where `N` is a runtime value
@psychocrypt psychocrypt force-pushed the topic-pedanticCompilerFlags branch from 686baf4 to 49b53ef Compare December 28, 2017 21:33
@psychocrypt psychocrypt changed the title pedantic and many more flags for GNU compiler [WIP] pedantic and many more flags for GNU compiler Dec 30, 2017
@psychocrypt
Copy link
Collaborator Author

I set this PR to WIP because I have still issues to get all compiling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants