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

Clang tidy auto refactor (braces, nullptr, and using) #1338

Closed

Conversation

SteveBronder
Copy link
Collaborator

Summary

Running clang-tidy with the checks below. This refactor is only aesthetic and does not change how any of the code is executed.

readability-braces-around-statements:

  • Adds braces around conditionals and ifs

modernize-use-using:
everywhere clang-tidy can replace a typedef with a using it will aka
typedef double A; becomes using A = double;

modernize-use-nullptr:
using nullptr instead of NULL

The original PR also had a cleanup to return a braced init list but after looking at the I didn't like it

Tests

refactor is only aesthetic so no new tests

Checklist

  • Math issue Update internals to use more modern c++ #1308

  • Copyright holder: Steve Bronder

    The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
    - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
    - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@rok-cesnovar
Copy link
Member

I dont understand this error.

git push https://****:****@github.com/bstatcomp/math.git clang-tidy-auto-refactor

remote: Permission to bstatcomp/math.git denied to ****.

fatal: unable to access 'https://****:****@github.com/bstatcomp/math.git/': The requested URL returned error: 403

This was never an issue before. Do we need to set something up on the fork or is Github acting up?

@SteveBronder
Copy link
Collaborator Author

I think we need to add the jenkinsbot user as a contributor to the bstat fork. I checked the 'allow edits from maintainers' thing was on but github must be goofing it for some reason

@rok-cesnovar
Copy link
Member

I gave stan-buidlbot write access but that invite needs to be confirmed. But yeah the maintainers thing should suffice. Weird.

@SteveBronder
Copy link
Collaborator Author

@serban-nicusor-toptal we just sent the stan buildbot an email for permissions so it can make pushes to branches on the bstatcomp fork, I think that would fix the error below?

++ clang-format --version
+ git commit -m '[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (tags/RELEASE_500/final)'
[clang-tidy-auto-refactor d68c93a] [Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (tags/RELEASE_500/final)
 81 files changed, 105 insertions(+), 105 deletions(-)
+ git push https://****:****@github.com/bstatcomp/math.git clang-tidy-auto-refactor
remote: Permission to bstatcomp/math.git denied to ****.
fatal: unable to access 'https://****:****@github.com/bstatcomp/math.git/': The requested URL returned error: 403
+ echo 'Exiting build because clang-format found changes.'
Exiting build because clang-format found changes.
+ echo 'Those changes are now found on stan-dev/math under branch clang-tidy-auto-refactor'
Those changes are now found on stan-dev/math under branch clang-tidy-auto-refactor
+ echo 'Please '\''git pull'\'' before continuing to develop.'
Please 'git pull' before continuing to develop.
+ exit 1
script returned exit code 1

@SteveBronder SteveBronder reopened this Sep 5, 2019
@serban-nicusor-toptal
Copy link
Contributor

@SteveBronder Hey, I don't have access to that email :(

@seantalts
Copy link
Member

seantalts commented Sep 5, 2019

I think doing this for the PR should solve it: https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Just a NB for all devs: the hope is that you all have clang-format and the githook (sh ./hooks/install_hooks.sh) installed locally so that commits are atomic and well-formatted as well as the resulting code.

@serban-nicusor-toptal serban-nicusor-toptal added this to the 3.0.0 milestone Oct 18, 2019
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.

4 participants