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

Changed all files to comply to code style checks. #691

Merged
merged 69 commits into from Apr 6, 2017

Conversation

@terhorstd
Contributor

terhorstd commented Mar 22, 2017

All files (except one) now comply to the code style checked by VERA++. The only non-compliance is due to a VERA-1.2.1 issue with pragma parallel for in nestkernel/connection_manager.cpp, which can be fixed by using VERA-1.3.1.

heplesser and others added some commits Mar 8, 2017

Modify vera++.profile to ignore problems with curly braces (rule T011…
…). The test can be confused by conditional code inclusion; missing or superfluous closing braces will be detected by the compiler and non-matching indentation will be fixed by clang-format.
Merge pull request #5 from stinebuu/fix_vera
Vera fixes on precise models
Merge pull request #6 from hakonsbm/fix_vera
Fixed Vera++ issues in topology
@heplesser

I will soon create a PR against @terhorstd with fixes for small issues I discovered.

Show outdated Hide outdated libnestutil/type_traits.h
Show outdated Hide outdated librandom/gamma_randomdev.cpp
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 23, 2017

Contributor

@terhorstd @jougs How do we handle the Vera++ problem in connection_manager.cpp? I first thought about installing Vera 1.3.0 on the test machine manually, but building it requires boost, and that would make the build process even tougher. Should we temporarily put connection_manager.cpp on the skip list for static code checks?

Contributor

heplesser commented Mar 23, 2017

@terhorstd @jougs How do we handle the Vera++ problem in connection_manager.cpp? I first thought about installing Vera 1.3.0 on the test machine manually, but building it requires boost, and that would make the build process even tougher. Should we temporarily put connection_manager.cpp on the skip list for static code checks?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 23, 2017

Contributor

@heplesser: I am not sure if it is such a good idea, if the authors of this PR are also its reviewers. Why don't we also draw in other people?

How can Travis signal green if there is still a problem? Is the solution in Travis 1.3.0 due to a proper fix or just an updated (Tcl/Tk) rule file? The latter could probably be installed without too much effort. I'm rather against adding such a central file to the skip list, as these lists are usually not regularly updated and the file might be forgotten there...

Contributor

jougs commented Mar 23, 2017

@heplesser: I am not sure if it is such a good idea, if the authors of this PR are also its reviewers. Why don't we also draw in other people?

How can Travis signal green if there is still a problem? Is the solution in Travis 1.3.0 due to a proper fix or just an updated (Tcl/Tk) rule file? The latter could probably be installed without too much effort. I'm rather against adding such a central file to the skip list, as these lists are usually not regularly updated and the file might be forgotten there...

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 24, 2017

Contributor

@jougs I will answer the technical questions first:

  • Travis build 1774 for NEST master, i.e., the one that shows green here, included only some 30 files in static code analysis. I am not sure why this happened, but it must have something to do with how we identify files that have changed. @gtrensch Could you take a look at this and create a new issue? Travis used Vera++ 1.2.1.
  • Travis build 410 for my NEST repo, on the other hand, checked more than 250 files (even though I only change a few files in my PR), and that included connection_manager.cpp, so that build failed.
  • I noticed that the current builds install boost as a Vera++-dependency anyways, so installing boost explicitly and then build Vera++ 1.3.0 manually on the Travis system would not be much extra burden.

Concerning who should review, I would consider this a two-tier thing in this case. This PR is massive, but changes are very mechanical, and have mostly been applied through project-wide regular expression substitution. One tedious but straightforward part of the review therefore is to scroll through heaps of code and check that {} ended up in the right places. I think most files have now been double-checked by people who have not (manually) added the same files, so we have a cross-check, though team-internal. I suggested this because I figured this would allow us to wade through a lot of code fast.

But we should probably invite a one or two high-level reviewers who look at the changes we made to infrastructure (travis.yml, vera++ profile, ...) and checks a smaller selection of files. Any suggestions on who to invite?

Contributor

heplesser commented Mar 24, 2017

@jougs I will answer the technical questions first:

  • Travis build 1774 for NEST master, i.e., the one that shows green here, included only some 30 files in static code analysis. I am not sure why this happened, but it must have something to do with how we identify files that have changed. @gtrensch Could you take a look at this and create a new issue? Travis used Vera++ 1.2.1.
  • Travis build 410 for my NEST repo, on the other hand, checked more than 250 files (even though I only change a few files in my PR), and that included connection_manager.cpp, so that build failed.
  • I noticed that the current builds install boost as a Vera++-dependency anyways, so installing boost explicitly and then build Vera++ 1.3.0 manually on the Travis system would not be much extra burden.

Concerning who should review, I would consider this a two-tier thing in this case. This PR is massive, but changes are very mechanical, and have mostly been applied through project-wide regular expression substitution. One tedious but straightforward part of the review therefore is to scroll through heaps of code and check that {} ended up in the right places. I think most files have now been double-checked by people who have not (manually) added the same files, so we have a cross-check, though team-internal. I suggested this because I figured this would allow us to wade through a lot of code fast.

But we should probably invite a one or two high-level reviewers who look at the changes we made to infrastructure (travis.yml, vera++ profile, ...) and checks a smaller selection of files. Any suggestions on who to invite?

Merge pull request #1 from heplesser/dennis-fix-vera-issues
reverted files that should not be fixed as they are from other sources.
@terhorstd

This comment has been minimized.

Show comment
Hide comment
@terhorstd

terhorstd Mar 24, 2017

Contributor

Regarding the the Vera++ problem in connection_manager.cpp, I think this goes away with the #define NEST_PARALLEL_FOR construct that @jougs and me discussed about. Do we have a PR for that already?

Contributor

terhorstd commented Mar 24, 2017

Regarding the the Vera++ problem in connection_manager.cpp, I think this goes away with the #define NEST_PARALLEL_FOR construct that @jougs and me discussed about. Do we have a PR for that already?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 24, 2017

Contributor

@terhorstd: here's the macro you've talked about:

#ifdef _OPENMP
  #define NEST_THREADED_LOOP_BEGIN(t) \
    _Pragma("omp parallel for") \
    { \
      size_t t = kernel().vp_manager.get_thread_id();
#else
  #define NEST_THREADED_LOOP_BEGIN(t) \
    for ( size_t t = 0; t < kernel().vp_manager.get_num_threads(); ++t ) \
    {
#endif

#define NEST_THREADED_LOOP_END }

Maybe you can really rename it to NEST_PARALLEL_FOR to be closer to the OpenMP original.

Contributor

jougs commented Mar 24, 2017

@terhorstd: here's the macro you've talked about:

#ifdef _OPENMP
  #define NEST_THREADED_LOOP_BEGIN(t) \
    _Pragma("omp parallel for") \
    { \
      size_t t = kernel().vp_manager.get_thread_id();
#else
  #define NEST_THREADED_LOOP_BEGIN(t) \
    for ( size_t t = 0; t < kernel().vp_manager.get_num_threads(); ++t ) \
    {
#endif

#define NEST_THREADED_LOOP_END }

Maybe you can really rename it to NEST_PARALLEL_FOR to be closer to the OpenMP original.

heplesser added a commit to heplesser/nest-simulator that referenced this pull request Mar 24, 2017

heplesser added a commit that referenced this pull request Mar 24, 2017

Merge pull request #694 from heplesser/deactivate-vera
Disabling VERA checks in master branch until #691 is merged. Merging immediately since it only disables VERA++ checks.
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 24, 2017

Contributor

@terhorstd With #694, I have disabled the Vera check in master so that we can merge PRs until this PR is merged, without having to apply style fixes to individual files. Could you merge that change from master and then enable the Vera-check again for this PR, so that the Vera-check will become permanently enabled once this PR is merged?

Contributor

heplesser commented Mar 24, 2017

@terhorstd With #694, I have disabled the Vera check in master so that we can merge PRs until this PR is merged, without having to apply style fixes to individual files. Could you merge that change from master and then enable the Vera-check again for this PR, so that the Vera-check will become permanently enabled once this PR is merged?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 6, 2017

Contributor

@jougs Would you agree to merging this now? After discussion with @terhorstd we leave the vera check inactive because there is still a problem with one line in connection_manager.cpp; this will be addressed either by #668 or #695, but this PR should not have to wait for them to avoid code rot.

Contributor

heplesser commented Apr 6, 2017

@jougs Would you agree to merging this now? After discussion with @terhorstd we leave the vera check inactive because there is still a problem with one line in connection_manager.cpp; this will be addressed either by #668 or #695, but this PR should not have to wait for them to avoid code rot.

@jougs

jougs approved these changes Apr 6, 2017

i did not check again in great detail, but agree that this should be merged now in order to avoid further problems.

@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch Apr 6, 2017

Contributor

@heplesser @jougs @terhorstd #668 will turn on the VERA check again. I would suggest the following procedure:

  1. merging this PR
  2. turning off VERA message checking, i.e. set IGNORE_MSG_VERA, in #668
  3. merging PR #668
  4. adding connection_manager.cpp to the exclusion list (don't know what the state of that is at the moment)
  5. provided that there are no messages, turn off IGNORE_MSG_VERA

(4) and (5) will be a seperate PR.

Contributor

gtrensch commented Apr 6, 2017

@heplesser @jougs @terhorstd #668 will turn on the VERA check again. I would suggest the following procedure:

  1. merging this PR
  2. turning off VERA message checking, i.e. set IGNORE_MSG_VERA, in #668
  3. merging PR #668
  4. adding connection_manager.cpp to the exclusion list (don't know what the state of that is at the moment)
  5. provided that there are no messages, turn off IGNORE_MSG_VERA

(4) and (5) will be a seperate PR.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 6, 2017

Contributor

@gtrensch I agree with nr 1-3 and eventually 5. Nr 4 may not be necessary, #695 probably will solve this issue in a much more elegant manner.

Contributor

heplesser commented Apr 6, 2017

@gtrensch I agree with nr 1-3 and eventually 5. Nr 4 may not be necessary, #695 probably will solve this issue in a much more elegant manner.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 6, 2017

Contributor

This PR fixes the formatting of a very large number of C++ files in the NEST code base, especially

  • placing all one-line blocks in curly braces
  • replacing ! with not in all boolean expressions
Contributor

heplesser commented Apr 6, 2017

This PR fixes the formatting of a very large number of C++ files in the NEST code base, especially

  • placing all one-line blocks in curly braces
  • replacing ! with not in all boolean expressions

@heplesser heplesser merged commit 49d9177 into nest:master Apr 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch Apr 6, 2017

Contributor

The IGNORE_MSG_VERA flag has been set. See PR #668.

Contributor

gtrensch commented Apr 6, 2017

The IGNORE_MSG_VERA flag has been set. See PR #668.

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