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

Remove usage of IVOS/regexp #2204

Merged
merged 49 commits into from
Jan 27, 2024
Merged

Remove usage of IVOS/regexp #2204

merged 49 commits into from
Jan 27, 2024

Conversation

alkino
Copy link
Member

@alkino alkino commented Jan 30, 2023 •
edited
Loading

Try to reduce the usage of InterViews outside of GUI.

Regexp was one of the IV classes that are now (since 10 years) in the standard.

Let's use standard, FTW!

EDIT: The engine inside IVOS is quite old and not really a standard one for today (and buggy), trying to replace with modern one leads to at least one problem: no support of std::multiline in standard libraries (except really new one: gcc 12, clang 15).

@azure-pipelines
Copy link

✔️ 9009929 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2023 •
edited by codecov bot
Loading

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c1e245d) 66.29% compared to head (bc73147) 66.30%.

❗ Current head bc73147 differs from pull request most recent head d3f783f. Consider uploading reports for the commit d3f783f to get more accurate results

Files Patch % Lines
src/ivoc/strfun.cpp 87.50% 4 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #2204   +/-   ##
=======================================
  Coverage   66.29%   66.30%           
=======================================
  Files         558      558           
  Lines      108430   108427    -3     
=======================================
+ Hits        71888    71893    +5     
+ Misses      36542    36534    -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@azure-pipelines
Copy link

✔️ 62ceafe -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@olupton
Copy link
Collaborator

olupton commented Jan 31, 2023

It would be great to drop this code, which is known to be buggy (#2008)...

From the comments, it looks like the old code aimed to support a "v8" dialect of regex, which some quick searching suggests might be more similar to egrep on https://en.cppreference.com/w/cpp/regex/syntax_option_type than the default ECMAScript? In any case, this will need to be tested extensively...

@bbpbuildbot

This comment has been minimized.

Sign in to view
@alkino
Copy link
Member Author

alkino commented Feb 1, 2023

std::regex::multiline (c++17) is available with GCC 12 and clang 15 only...

@alkino
Copy link
Member Author

alkino commented Feb 1, 2023 •
edited
Loading 

$ gr "sfunc.\bhead\b" | wc -l
> 897

@alkino
Copy link
Member Author

alkino commented Feb 1, 2023 

" "
"/"
"="
">"
"[ :]"
"\""
"\\."
"\\["
"\\]"
"/[^/]+$"
"[^/]+$"
"[^-+0-9.e]"
"\.[_a-z]+$"
"\.[_A-Za-z0-9]+$"
"//[^b]"
"//b[1-9]"
"\n"

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ cf8c258 -> Azure artifacts URL

@olupton olupton mentioned this pull request Jun 5, 2023
Closed
@olupton olupton mentioned this pull request Jun 22, 2023
Open
@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines
Copy link

✔️ d55fb9e -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines Azure Pipelines
Copy link

✔️ f98ebed -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@alkino alkino marked this pull request as ready for review January 26, 2024 10:23
@azure-pipelines Azure Pipelines
Copy link

✔️ 3eef6d3 -> Azure artifacts URL

@github-actions GitHub Actions
Copy link
Contributor

NEURON ModelDB CI: launching for 3eef6d3 via its drop url

@bbpbuildbot

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

NEURON ModelDB CI: 3eef6d3 -> download reports from here

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines Azure Pipelines
Copy link

✔️ 24404d5 -> Azure artifacts URL

@github-actions GitHub Actions
Copy link
Contributor

NEURON ModelDB CI: launching for 24404d5 via its drop url

@github-actions GitHub Actions
Copy link
Contributor

NEURON ModelDB CI: 24404d5 -> download reports from here

@alkino
Copy link
Member Author

alkino commented Jan 27, 2024

nrn-modeldb-ci got no difference

pramodk
pramodk approved these changes Jan 27, 2024
View reviewed changes
Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

@nrnhines : if I am not mistaken, you have already checked these changes while adding the test in 628a77a.

As all tests and ModelDB passes, does this look good to you for merge?

@azure-pipelines Azure Pipelines
Copy link

✔️ bc73147 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
nrnhines
nrnhines approved these changes Jan 27, 2024
View reviewed changes
Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

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

Glad this is replaced by a standard

@alkino alkino enabled auto-merge (squash) January 27, 2024 16:08
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 27, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@azure-pipelines Azure Pipelines
Copy link

✔️ d3f783f -> Azure artifacts URL

@alkino alkino merged commit 128c684 into master Jan 27, 2024
34 of 35 checks passed
@alkino alkino deleted the cornu/remove_regexp branch January 27, 2024 16:55
@bbpbuildbot
Copy link
Collaborator

Logfiles from GitLab pipeline #188754 (:no_entry:) have been uploaded here!

Status and direct links:

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

@pramodk pramodk pramodk approved these changes

@nrnhines nrnhines nrnhines approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alkino @bbpbuildbot @codecov-commenter @olupton @pramodk @nrnhines