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

remove gsl-lite dependency #3225

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

Conversation

gridley
Copy link
Contributor

@gridley gridley commented Dec 15, 2024

Description

The C++ GSL does barely anything. Span is nice, but it's barely any effort to just make your own. Less dependencies is nice to have. I have had one issue requiring manual reconfiguration of GSL lite before when building openmc, so this will reduce the chance of others running into that.

Also, assert is arguably superior to Expects and Ensures provided by GSL. If we want, we can do #define Expects assert. But it's very nice to know many checks can be added with assert that vanish when compiling outside debug mode.

Lastly, since gsl::index is typedef'd to an int64_t 99.999999% of the time on modern computers, it replaced all instances with that. This would actually change behavior in certain cases on 32 bit computers, but using the 64 bit version can only help.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • [] I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@gridley
Copy link
Contributor Author

gridley commented Dec 15, 2024

cpp-linter is really not great. It has false positives almost every time a change is made due to slight discrepancies in how different clang-format versions interpret things. I have gone through trying different clang-formats and still can't make it happy. It says to use version 15, but I can't find any straightforward way to install this on my mac.

I will have to return later to figure out what it's fussing about.

@jtramm
Copy link
Contributor

jtramm commented Dec 16, 2024

This is awesome -- thanks for removing this dependency!

When I run clang format with LLVM 15.0.0 or LLVM 18.1.0, I get the following diff:

diff --git a/src/mesh.cpp b/src/mesh.cpp
index 9ac60c864..2cb081bfd 100644
--- a/src/mesh.cpp
+++ b/src/mesh.cpp
@@ -3016,7 +3016,7 @@ void LibMesh::initialize()
     bin_to_elem_map_.reserve(m_->n_active_elem());
     elem_to_bin_map_.resize(m_->n_elem(), -1);
     for (auto it = m_->active_elements_begin(); it != m_->active_elements_end();
-      it++) {
+         it++) {
       auto elem = *it;
 
       bin_to_elem_map_.push_back(elem->id());
@@ -3175,7 +3175,7 @@ void LibMesh::set_score_data(const std::string& var_name,
   unsigned int std_dev_num = variable_map_.at(std_dev_name);
 
   for (auto it = m_->local_elements_begin(); it != m_->local_elements_end();
-    it++) {
+       it++) {
     if (!(*it)->active()) {
       continue;
     }
lines 1-22/22 (END)

Seems like LLVM 19 made some change to behavior here unfortunately.

@jtramm
Copy link
Contributor

jtramm commented Dec 16, 2024

I was able to push a fix for the formatting issue, but now it seems to be hitting a compile time issue when MPI is enabled. Could be an issue with some of the spans being changed to const types?

@gridley
Copy link
Contributor Author

gridley commented Dec 16, 2024

Thanks for the fix John! I'll be able to fix that in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants