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

Inconsistent formatting since clang-format 9 merge #2276

Closed
mlober opened this issue Jan 26, 2022 · 5 comments · Fixed by #2279
Closed

Inconsistent formatting since clang-format 9 merge #2276

mlober opened this issue Jan 26, 2022 · 5 comments · Fixed by #2279
Labels
S: High Should be handled next T: Bug Wrong statements in the code or documentation

Comments

@mlober
Copy link
Contributor

mlober commented Jan 26, 2022

Describe the bug
Found files with inconsistent formatting:

MSGBLD0220: + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
MSGBLD0220: +                 STATIC CODE ANALYSIS DETECTED PROBLEMS !                    +
MSGBLD0220: + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +

MSGBLD0220: C/C++ files with formatting errors:
MSGBLD0220: ... libnestutil/block_vector.h
MSGBLD0220: ... libnestutil/enum_bitfield.h
MSGBLD0220: ... libnestutil/lockptr.h
MSGBLD0220: ... libnestutil/sort.h
MSGBLD0220: ... nestkernel/connection_creator.cpp
MSGBLD0220: ... nestkernel/connection_manager.cpp
MSGBLD0220: ... nestkernel/event.h
MSGBLD0220: ... nestkernel/grid_layer.h
MSGBLD0220: ... nestkernel/nest_time.h
MSGBLD0220: ... nestkernel/node_collection.h
MSGBLD0220: ... nestkernel/ntree.h
MSGBLD0220: ... nestkernel/position.h
MSGBLD0220: ... nestkernel/stimulation_backend_mpi.cpp
MSGBLD0220: ... nestkernel/target_data.h
MSGBLD0220: ... sli/dict.cc
MSGBLD0220: ... sli/dict.h
MSGBLD0220: ... sli/tarrayobj.h
MSGBLD0220: ... sli/token.h
MSGBLD0220: ... sli/tokenarray.h
Error: Process completed with exit code 1.

To Reproduce
Steps to reproduce the behavior:
checkout to master and run clang-format on any of the files above.
in terminal: diff -u nestkernel/node_collection.h /dev/stdin <<<$(clang-format nestkernel/node_collection.h)

Expected behavior
No difference when running clang-format on the (untouched) files above.

Desktop/Environment (please complete the following information):

  • OS Ubuntu 20.04
  • Python-Version: 3.9
  • NEST-Version: master
  • Installation: nest-simulator conda environment; apt-installed clang-format version 10 (although this is the wrong version, formatting for example nestkernel/node_collection.h with clang-format 10 fixed the issue for this file.)

Additional context
see static tests failing here: https://github.com/mlober/nest-simulator/runs/4941813778?check_suite_focus=true

After fixing the formatting of one of the files, the static checks on github ran perfectly, even though the other files were still broken --> The testsuit seems to only check for changes (?)

@terhorstd terhorstd added S: High Should be handled next T: Bug Wrong statements in the code or documentation labels Jan 26, 2022
@heplesser
Copy link
Contributor

I noticed that the build log shows that clang-format-11 is used on Github: https://github.com/mlober/nest-simulator/runs/4941813778?check_suite_focus=true#step:9:4002

Since different clang-format versions have slightly different built-in rules, this might be the problem. Which version of clang-format did you use for the local check on your computer?

@jougs Could the problem be that ci_build.sh no longer requires the version specifically, so if a clang-format is availanle somewhere, that will be used (

CLANG_FORMAT=clang-format
)?

@mlober
Copy link
Contributor Author

mlober commented Jan 26, 2022

On my computer, I checked with clang-format version 10 and fixed one of the files. After pushing it, the checks on github worked again. So for this particular file at least, the formatting of clang-format 10 and 11 (as you said) should be the same.

@heplesser
Copy link
Contributor

Our logic concerning clang-format is different: The code should conform to clang-format-9. We just need to fix the way we run it on Github.

@mlober
Copy link
Contributor Author

mlober commented Jan 26, 2022

I see the issue with the files. Some functions in them are formatted to the new standard and some are not.
For example, this line should be formatted into two lines (just like all the other functions above it):

inline Time operator*( const long factor, const Time& t )

@jougs
Copy link
Contributor

jougs commented Jan 27, 2022

@terhorstd and myself are working towards a solution, where each static code analysis check is its own job and all files are checked continuously instead of only the changed ones. That should eliminate the problem of new changes shadowing formatting errors in previously made changes, but will still take a bit of time until it's in a mergeable state.

Regarding the issue of not specifying the version in clang-format in our build-scripts on the CI, I created #2279 that again nails down the version of clang-format we use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
Development

Successfully merging a pull request may close this issue.

4 participants