Skip to content

Remove IFERROR / MODEL_LEVEL / THREADSAFE list#2005

Merged
alexsavulescu merged 7 commits into
masterfrom
remove_iferror
Oct 4, 2022
Merged

Remove IFERROR / MODEL_LEVEL / THREADSAFE list#2005
alexsavulescu merged 7 commits into
masterfrom
remove_iferror

Conversation

@alkino

@alkino alkino commented Oct 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

@nrnhines can you do a deep review for solve_queue and explicit_decl because logic was quiet hard to understand for me. I hope I did the best, but I'm not sure.

Sorry

See #1957

@azure-pipelines

azure-pipelines Bot commented Oct 1, 2022

Copy link
Copy Markdown

✔️ 7a5c9f72e1ea79d88f90a53083e4cc0d03e777c6 -> Azure artifacts URL

Base automatically changed from remove_match_reset_forall to master October 2, 2022 07:40
@codecov-commenter

codecov-commenter commented Oct 2, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2005 (fed8569) into master (8a33d04) will increase coverage by 0.00%.
The diff coverage is 90.62%.

@@           Coverage Diff           @@
##           master    #2005   +/-   ##
=======================================
  Coverage   47.50%   47.51%           
=======================================
  Files         527      527           
  Lines      118952   118886   -66     
=======================================
- Hits        56512    56483   -29     
+ Misses      62440    62403   -37
Impacted Files Coverage Δ
src/modlunit/init.cpp 100.00% <ø> (ø)
src/modlunit/symbol.cpp 96.49% <ø> (-0.07%) ⬇️
src/nmodl/discrete.cpp 5.88% <ø> (ø)
src/nmodl/init.cpp 100.00% <ø> (ø)
src/nmodl/list.cpp 96.36% <ø> (ø)
src/nmodl/modl.cpp 67.92% <ø> (ø)
src/nmodl/nocpout.cpp 94.72% <0.00%> (+0.10%) ⬆️
src/nmodl/symbol.cpp 100.00% <ø> (ø)
src/nmodl/parsact.cpp 82.80% <80.00%> (+2.37%) ⬆️
src/nmodl/solve.cpp 67.87% <95.83%> (-0.11%) ⬇️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@alkino alkino force-pushed the remove_iferror branch 2 times, most recently from 0369543 to 3d6f74e Compare October 2, 2022 09:26
@azure-pipelines

azure-pipelines Bot commented Oct 2, 2022

Copy link
Copy Markdown

✔️ 3d6f74e0d01b939507562ffd984b551218f9cd03 -> Azure artifacts URL

nrnhines
nrnhines reviewed Oct 2, 2022
View reviewed changes

@nrnhines nrnhines left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks basically ok. There are a few places where more removal can take place. I'm a bit confused whether or not there is any substantive need for Symbol.level. If so, perhaps it could use a name change.

Comment thread src/modlunit/parse1.ypp Outdated
Comment thread src/modlunit/parse1.ypp Outdated
Comment thread src/modlunit/symbol.cpp Outdated
Comment thread src/nmodl/parsact.cpp Outdated
Comment thread src/nmodl/parsact.cpp Outdated
Comment thread src/nmodl/parsact.cpp Outdated
Comment thread src/nmodl/parsact.cpp Outdated
Comment thread src/nmodl/symbol.cpp Outdated
@alkino

alkino commented Oct 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

I'm sorry @nrnhines but I think you will have to fix level yourself I'm trying but totally lost in code.

I found a new place with: src/modlunit/declare.cpp

@nrnhines

nrnhines commented Oct 4, 2022

Copy link
Copy Markdown
Member

I'm sorry @nrnhines but I think you will have to fix level yourself I'm trying but totally lost in code.

will work on this

@nrnhines

nrnhines commented Oct 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

looks like we've stepped on each others toes with 44e2a6cd0. I was just getting ready to push Old nmodl merge code removed (never really functional). I've merged my changes with your and will push in a few minutes.

@alexsavulescu

alexsavulescu commented Oct 4, 2022

Copy link
Copy Markdown
Member

@nrnhines worth running nrn-modeldb-ci over this PR ?

@nrnhines

nrnhines commented Oct 4, 2022

Copy link
Copy Markdown
Member

@nrnhines worth running nrn-modeldb-ci over this PR ?

That would be a good idea.

@azure-pipelines

azure-pipelines Bot commented Oct 4, 2022

Copy link
Copy Markdown

✔️ fed8569 -> Azure artifacts URL

alexsavulescu
alexsavulescu reviewed Oct 4, 2022
View reviewed changes

@alexsavulescu alexsavulescu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nrn-modeldb-ci: https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/3181158338

alexsavulescu
alexsavulescu reviewed Oct 4, 2022
View reviewed changes
Comment thread src/modlunit/declare.cpp
@alexsavulescu alexsavulescu merged commit 0cdcc5c into master Oct 4, 2022
@alexsavulescu alexsavulescu deleted the remove_iferror branch October 4, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nrnhines nrnhines nrnhines approved these changes
@alexsavulescu alexsavulescu alexsavulescu left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alkino @codecov-commenter @nrnhines @alexsavulescu