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

Add Guidelines Support Library #1394

Merged
merged 1 commit into from Jan 8, 2016

Conversation

Projects
None yet
5 participants
@vlj
Copy link
Contributor

commented Jan 3, 2016

See #1392 for more details.

The PR adds GSL project as a submodule and add it to stdafx.h for future use.

@mention-bot

This comment has been minimized.

Copy link

commented Jan 3, 2016

By analyzing the blame information on this pull request, we identified @Nekotekina, @danilaml and @Bigpet to be potential reviewers

@vlj vlj force-pushed the vlj:build branch 5 times, most recently from 7582148 to bf8ec12 Jan 3, 2016

@vlj

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2016

Rebased, fixed cmake build and added submodule to appveyor/travis.
Also only include it in DX12 for now.

@vlj

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2016

Waiting for review/merge

@danilaml

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2016

@vlj but can VS2015 check these guidelines as of now? With help of this lib of course. Or newer clang? Maybe it's better to enable these on travis too.

@vlj

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2016

You need to download from NuGet 2 tools, cppcorecheck and gsl (search with "static analysis" keyword).
Unfortunately they're not compatible with shared pch (as static analysis) but it's possible to manually disable pch and run the tools. They're still work in progress and they crash on d3d12gsrender analysis now.
I don't know if we should enable the tools for travis. It will print several warnings in the log and they're likely a lot of them. We should fix all the errors emitted by clang before enabling them by default.
Meanwhile it can be run manually on part of project.

Please note that while GSL helps static tools, c++ type system already ensures some bounds and type checking happening at compile time and runtime with as least redundancy as possible. For instance the span template ensures that there is no buffer overflow happening when used naked pointers (which are common since we deal with memory).

@vlj

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2016

can it be merged ?

@paulsapps

This comment has been minimized.

Copy link

commented Jan 4, 2016

IMO the shared pch is a hack anyway

@danilaml

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2016

@vlj is seems rather raw right now, with errors such as these: microsoft/GSL@d38e621 (might as well update submodule to latest commit) still appearing. Do you intend to start using its classes and converting old code right away?
@paulsapps @Nekotekina insisted on using them instead of separate pchs.

@vlj

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2016

In my opinion the sooner we start to write (new) code with this template the better since we won't have to port it later. That's why I'd like to use them in d3d12 backend, I plan to do some refactoring in render target handling code and that would be a good opportunity to use GSL.

@vlj vlj force-pushed the vlj:build branch 2 times, most recently from 9a497e7 to 8526fe0 Jan 5, 2016

@vlj vlj force-pushed the vlj:build branch from 8526fe0 to 1ce7269 Jan 7, 2016

@vlj

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2016

Rebased + updated gsl head + updated gcc in travis-ci since GSL requires at least version 5.1.

@danilaml

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2016

@vlj I guess we don't really care about compiler compatibility that much, if you really think we could use GSL to our advantage. Readme needs to be updated with min requirements too.

@vlj vlj force-pushed the vlj:build branch 3 times, most recently from 528f692 to 375013b Jan 7, 2016

@vlj

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2016

Rebased + updated Readme

@vlj vlj force-pushed the vlj:build branch from 375013b to e759143 Jan 7, 2016

vlj added a commit that referenced this pull request Jan 8, 2016

Merge pull request #1394 from vlj/build
Add Guidelines Support Library

@vlj vlj merged commit 9d1208b into RPCS3:master Jan 8, 2016

3 checks passed

code-review/pullapprove Approved by vlj
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.