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 9 commits into
base: develop
Choose a base branch
from
Open

refactor: address some code analysis issues #3456

wants to merge 9 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
paveltomin commented Nov 20, 2024
View reviewed changes
src/coreComponents/dataRepository/xmlWrapper.cpp
@@ -429,7 +429,6 @@ size_t findAttribute( string const & attName, string const & xmlBuffer, size_t c
{
return candidatePos;
}
searchStart = candidatePos + attName.size();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not used

paveltomin
paveltomin commented Nov 21, 2024
View reviewed changes
src/coreComponents/fileIO/timeHistory/HistoryCollectionBase.cpp
@@ -189,9 +189,6 @@ dataRepository::Group const * HistoryCollectionBase::getTargetObject( DomainPart
}
else
{
string const targetTokensStr = stringutilities::join( targetTokens.begin(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not used

@paveltomin paveltomin changed the title refactor: fix address some code analysis issues refactor: address some code analysis issues Nov 21, 2024
@paveltomin paveltomin removed the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Nov 21, 2024
MelReyCG
MelReyCG approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@MelReyCG MelReyCG left a comment

Choose a reason for hiding this comment

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

Nice!

...ents/physicsSolvers/fluidFlow/kernels/compositional/SolutionScalingAndCheckingKernelBase.hpp
@@ -84,6 +84,7 @@ class SolutionScalingAndCheckingKernelBase

StackVariables( real64 _localMinVal )
:
localRow( -1 ),
Copy link
Contributor

@MelReyCG MelReyCG Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just a question: why -1 instead of 0? (=> localRow())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-1 to force it to crash when something goes wrong

src/coreComponents/physicsSolvers/multiphysics/HydrofractureSolver.cpp
Comment on lines +406 to +407
dispMeshTargets[std::make_pair( meshBodyName, solidDiscretizationName )] = regions;
presMeshTargets[std::make_pair( meshBodyName, flowDiscretizationName )] = regions;
Copy link
Contributor

Choose a reason for hiding this comment

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

How did the previous code worked?? Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't think using moved is a big deal here but better not to use it

@paveltomin
Copy link
Contributor Author

@CusiniM, @acitrain, @castelletto1, @frankfeifan, @jhuang2601, @rrsettgast, @ryar9534, @sframba, @untereiner, @wrtobin please approve

@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/finiteElement/elementFormulations/ConformingVirtualElementOrder1.hpp Outdated Show resolved Hide resolved
src/coreComponents/constitutive/ExponentialRelation.hpp
@@ -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?

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

@rrsettgast rrsettgast rrsettgast requested changes

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

@MelReyCG MelReyCG MelReyCG approved these changes

@acitrain acitrain acitrain approved these changes

@jhuang2601 jhuang2601 jhuang2601 approved these changes

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

@ryar9534 ryar9534 Awaiting requested review from ryar9534 ryar9534 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

@dkachuma dkachuma Awaiting requested review from dkachuma dkachuma 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 matteofrigo5 is a code owner

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

Requested changes must be addressed to merge this pull request.

Assignees

@paveltomin paveltomin

Labels
flag: no rebaseline Does not require rebaseline flag: ready for review type: cleanup / refactor Non-functional change (NFC)
Projects
None yet
Milestone
No milestone
Development

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