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.

Sign up for GitHub

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

Jump to bottom

refactor: address some code analysis issues #3456

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from
Open

refactor: address some code analysis issues #3456

wants to merge 25 commits into from

Conversation

paveltomin
Copy link
Contributor

No description provided.

@paveltomin paveltomin self-assigned this Nov 20, 2024
paveltomin
paveltomin commented Nov 20, 2024
View reviewed changes
src/coreComponents/constitutive/solid/ElasticIsotropicPressureDependent.hpp
@@ -337,7 +337,7 @@ void ElasticIsotropicPressureDependentUpdates::smallStrainUpdate( localIndex con
real64 eps_v_elastic;
real64 oldElasticStrainVol;
real64 oldElasticStrainDev;
real64 bulkModulus = -p0/Cr;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reassigned below, down in the code

@paveltomin paveltomin marked this pull request as ready for review November 20, 2024 16:18
@paveltomin paveltomin added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Nov 20, 2024
@paveltomin paveltomin added type: cleanup / refactor Non-functional change (NFC) flag: ready for review flag: no rebaseline Does not require rebaseline labels Nov 20, 2024
@paveltomin paveltomin changed the title fix: address some code analysis issues refactor: fix address some code analysis issues Nov 20, 2024
@paveltomin
Copy link
Contributor Author

@CusiniM, @andrea-borio, @castelletto1, @dkachuma, @frankfeifan, @jhuang2601, @rrsettgast, @ryar9534, @untereiner, and/or @wrtobin please approve

rrsettgast
rrsettgast requested changes Dec 12, 2024
View reviewed changes
src/coreComponents/constitutive/ExponentialRelation.hpp Outdated
@@ -421,19 +421,19 @@ class ExponentialRelation
private:

/// variable shift
T m_x0;
T m_x0{};
Copy link
Member

Choose a reason for hiding this comment

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

there is a bit of a structural problem here. We have constructors, but none of these variables are initialized there. They are set in functions called from within the constructor....but that is a different thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so how should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rrsettgast please let me know what to fix?

@paveltomin paveltomin requested a review from rrsettgast January 8, 2025 19:35
@paveltomin paveltomin requested a review from tjb-ltk as a code owner February 7, 2025 22:29
@paveltomin
Copy link
Contributor Author

@rrsettgast please let me know what changes are requested

@paveltomin paveltomin removed the request for review from ryar9534 March 4, 2025 15:33
rrsettgast
rrsettgast approved these changes Mar 6, 2025
View reviewed changes
Copy link
Member

@rrsettgast rrsettgast left a comment

Choose a reason for hiding this comment

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

Just initialize these to either:

  • some value like 0
  • an invalid value that will trigger an error and requires them to be set manually

@paveltomin paveltomin added ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI and removed flag: ready for review labels Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rrsettgast rrsettgast rrsettgast approved these changes

@jhuang2601 jhuang2601 jhuang2601 approved these changes

@acitrain acitrain acitrain approved these changes

@dkachuma dkachuma dkachuma approved these changes

@MelReyCG MelReyCG MelReyCG approved these changes

@Guotong-Ren Guotong-Ren Guotong-Ren approved these changes

@CusiniM CusiniM Awaiting requested review from CusiniM CusiniM is a code owner

@wrtobin wrtobin Awaiting requested review from wrtobin wrtobin is a code owner

@untereiner untereiner Awaiting requested review from untereiner untereiner is a code owner

@corbett5 corbett5 Awaiting requested review from corbett5 corbett5 is a code owner

@castelletto1 castelletto1 Awaiting requested review from castelletto1 castelletto1 is a code owner

@sframba sframba Awaiting requested review from sframba sframba is a code owner

@frankfeifan frankfeifan Awaiting requested review from frankfeifan frankfeifan is a code owner

@matteofrigo5 matteofrigo5 Awaiting requested review from matteofrigo5

@andrea-borio andrea-borio Awaiting requested review from andrea-borio andrea-borio is a code owner

@tjb-ltk tjb-ltk Awaiting requested review from tjb-ltk tjb-ltk is a code owner

Assignees

@paveltomin paveltomin

Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: no rebaseline Does not require rebaseline type: cleanup / refactor Non-functional change (NFC)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@paveltomin @rrsettgast @jhuang2601 @acitrain @dkachuma @MelReyCG @Guotong-Ren