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

Modernize the codebase - remove unused imports #2490

Merged
merged 11 commits into from
Oct 13, 2022

Conversation

JanVogelsang
Copy link
Contributor

This PR removes all unused imports in C/C++ files.

@JanVogelsang JanVogelsang changed the title Unused imports Remove unused imports Oct 4, 2022
@JanVogelsang JanVogelsang marked this pull request as draft October 4, 2022 12:09
@JanVogelsang
Copy link
Contributor Author

Can someone support me with the failing checks? Somehow no errors got reported and all checks pass, but still the pipeline exits with an error code of 1.

@heplesser
Copy link
Contributor

Can someone support me with the failing checks? Somehow no errors got reported and all checks pass, but still the pipeline exits with an error code of 1.

The problem are the compiler warnings:

Make                   | Failed                             |
[2600](https://github.com/nest/nest-simulator/actions/runs/3182211083/jobs/5187939958#step:9:2601)
|                        |                                    |
[2601](https://github.com/nest/nest-simulator/actions/runs/3182211083/jobs/5187939958#step:9:2602)
|                        | Errors  : 0                        |
[2602](https://github.com/nest/nest-simulator/actions/runs/3182211083/jobs/5187939958#step:9:2603)
|                        | Warnings: 4

Known warnings that are to be ignored are defined here

known_warnings = [
f'{build_dir}/sli/scanner.cc:638:13: warning: this statement may fall through [-Wimplicit-fallthrough=]',
f'{build_dir}/sli/scanner.cc:668:19: warning: this statement may fall through [-Wimplicit-fallthrough=]',
f'{build_dir}/sli/scanner.cc:710:13: warning: this statement may fall through [-Wimplicit-fallthrough=]',
f'{build_dir}/sli/scanner.cc:738:24: warning: this statement may fall through [-Wimplicit-fallthrough=]',

with explicit line numbers. Since you removed one include line from sli/scanner.cc, the line numbers need to be updated.

@JanVogelsang JanVogelsang marked this pull request as ready for review October 4, 2022 21:08
@nest nest deleted a comment from JanVogelsang Oct 5, 2022
@JanVogelsang JanVogelsang marked this pull request as draft October 6, 2022 18:55
@JanVogelsang JanVogelsang marked this pull request as ready for review October 6, 2022 19:16
@JanVogelsang JanVogelsang changed the title Remove unused imports Modernizing the codebase - removing unused imports Oct 10, 2022
@jougs jougs requested review from heplesser and jougs October 11, 2022 15:43
@jougs jougs added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Oct 11, 2022
@jougs jougs added this to PRs in progress in Kernel via automation Oct 11, 2022
@jougs jougs requested review from jougs and removed request for jougs October 11, 2022 15:44
@jougs
Copy link
Contributor

jougs commented Oct 11, 2022

@JanVogelsang: Can you please fix the conflict? Thanks!

@JanVogelsang
Copy link
Contributor Author

Well, one of the checks failed because of an unstable connection, as it seems. Is there any way for me to rerun the checks? The tests pass on my branch, so they will also pass here.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one little fix, see inline.

nestkernel/event_delivery_manager.cpp Show resolved Hide resolved
Kernel automation moved this from PRs in progress to PRs pending approval Oct 11, 2022
@jougs
Copy link
Contributor

jougs commented Oct 12, 2022

@clinssen, @pnbabu: Can you please check if this PR has consequences for NESTML? And this triggers a question: are includes in the NESTML templates added conditionally depending on what features are actually used in the model or is there just a bunch of default includes?

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have these changes been suggested by a tool? If so, could this be made into a static code check that runs on the CI to reject commits that are adding unused includes? I'd be willing to wait for this PR until such a test is added.

@heplesser
Copy link
Contributor

Have these changes been suggested by a tool? If so, could this be made into a static code check that runs on the CI to reject commits that are adding unused includes? I'd be willing to wait for this PR until such a test is added.

Good point! Obviously, it needs to be a free open source tool.

@JanVogelsang
Copy link
Contributor Author

JanVogelsang commented Oct 12, 2022

I didn't use an open-source tool for this task, however I found one on GitHub, that seems to do the same thing: include what you use

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heplesser: Does your approval mean that we skip the addition of the static test for now? @JanVogelsang: can you please create an issue to remind us of the addition of a such a test?

I'm approving now, as I'm fine with the changes in this PR.

@heplesser
Copy link
Contributor

@jougs Yes, I think it is better to get these changes in place and add the automated check later through a separate issue. I will merge now.

@heplesser heplesser merged commit f7ece1a into nest:master Oct 13, 2022
Kernel automation moved this from PRs pending approval to Done (PRs and issues) Oct 13, 2022
@JanVogelsang JanVogelsang deleted the refactoring-unused-imports branch October 13, 2022 10:32
@jougs jougs changed the title Modernizing the codebase - removing unused imports Modernize the codebase - remove unused imports Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Kernel
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants