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

Code cleanup (using namespace std) #260

Merged
merged 35 commits into from
Oct 16, 2023
Merged

Code cleanup (using namespace std) #260

merged 35 commits into from
Oct 16, 2023

Conversation

HomesGH
Copy link
Contributor

@HomesGH HomesGH commented May 24, 2023

Description

This PR deletes all using namespace std;. Therefore, std:: functions (e.g., string, array, map, ...) had to be declared explicitly.
Check the changes with the following bash commands:

Deleted in src:
git diff --word-diff=porcelain --word-diff-regex=. origin/master HEAD src | grep "^-" | grep -v "\-\-\-" | sort | uniq -c

Added in src:
git diff --word-diff=porcelain --word-diff-regex=. origin/master HEAD src | grep "^+" | grep -v "+++" | sort | uniq -c

Replace src with tools to get the same output for the tools folder. Omit it for changes in the whole project.

Most of the "weird" strings like ng, f or if (isnan arise from line 1258 in Simulation.cpp.

This PR also adds some static code analysis to prevent the usage of using namespace std; in the future.

Related Pull Requests

Resolved Issues

How Has This Been Tested?

Some check scripts where added which build the code with different options (but not combined). All runs succeeded.

Open issues:

  • Static code analysis has to be enabled in GitHub Actions
  • Some cmake options seem to be not working and are not provided by ccmake. E.g. FMM_FFT, QUICKSCHED (maybe set indirectly?) and WITH_PAPI
  • using Log is used multiple times -> solved in discussion (see below)

tools/APE/data/FileStruct.py Fixed Show fixed Hide fixed
tools/APE/data/SettingStruct.py Fixed Show fixed Hide fixed
@HomesGH HomesGH requested review from FG-TUM and amartyads May 24, 2023 20:02
@HomesGH HomesGH added the CI label May 24, 2023
@HomesGH HomesGH changed the title Code cleanup Code cleanup (using namespace std) May 24, 2023
tools/APE/data/FileStruct.py Fixed Show fixed Hide fixed
tools/APE/data/SettingStruct.py Fixed Show fixed Hide fixed
Copy link
Contributor

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Contributor

@amartyads amartyads left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, but the using Logger thing is again weird. Do we have bleedthrough of that too, or do we not need that in files where we already have #include utils/Logger.h"?

src/Simulation.h Outdated Show resolved Hide resolved
tools/APE/data/SettingStruct.py Outdated Show resolved Hide resolved
src/ensemble/CavityEnsemble.cpp Show resolved Hide resolved
checks/run-staticAnalysis.sh Outdated Show resolved Hide resolved
checks/run-staticAnalysis.sh Outdated Show resolved Hide resolved
@HomesGH HomesGH requested a review from FG-TUM June 12, 2023 10:13
checks/run-staticAnalysis.sh Outdated Show resolved Hide resolved
@HomesGH HomesGH requested review from FG-TUM and amartyads October 10, 2023 10:38
Copy link
Member

@FG-TUM FG-TUM left a comment

Choose a reason for hiding this comment

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

I carefully approve but would like to have a second opinion before merging. @amartyads

@amartyads
Copy link
Contributor

I also tentatively approve from my side

@FG-TUM FG-TUM merged commit f3a8ba4 into master Oct 16, 2023
52 checks passed
@FG-TUM FG-TUM deleted the codeCleanup branch October 16, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants