Skip to content

fix the remaining warnings in the codebase #123

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

Closed
wants to merge 3 commits into from
Closed

fix the remaining warnings in the codebase #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a79604
address the remaining warnings resulting from the new linting flags
superwhiskers Jun 2, 2025
471286a
remove unnecessary comment in the root `CMakeLists.txt` now that we'v…
superwhiskers Jun 2, 2025
12fa7be
finish making all implicit casts explicit
superwhiskers Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ set(PACKAGE_VERSION_PATCH "0")

set(PACKAGE_VERSION "${PACKAGE_VERSION_MAJOR}.${PACKAGE_VERSION_MINOR}.${PACKAGE_VERSION_PATCH}")

# TODO: Probably beter to set a debug interface target
# set(CMAKE_CXX_FLAGS_DEBUG "-Wall -O0 -g -DDEBUG")

set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_FLAGS "-Wall -Wextra -Wconversion -Wpedantic")

Expand Down
2 changes: 1 addition & 1 deletion examples/PowerElectronics/Microgrid/Microgrid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ int main(int /* argc */, char const** /* argv */)
dg1->setExternalConnectionNodes(1, dqbus1);
dg1->setExternalConnectionNodes(2, dqbus1 + 1);
//"grounding" of the difference
dg1->setExternalConnectionNodes(3, -1);
dg1->setExternalConnectionNodes(3, static_cast<size_t>(-1));
// internal connections
for (size_t i = 0; i < 12; i++)
{
Expand Down
4 changes: 2 additions & 2 deletions examples/PowerElectronics/RLCircuit/RLCircuit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ int main(int /* argc */, char const** /* argv */)
// input
induct->setExternalConnectionNodes(0, 1);
// output
induct->setExternalConnectionNodes(1, -1);
induct->setExternalConnectionNodes(1, static_cast<size_t>(-1));
Copy link
Collaborator

@nkoukpaizan nkoukpaizan Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is this conversion meaningful? This will store the largest integer, right? It works because the same casting is done for the variable this is compared to, e.g.,

GridKit/src/Model/PowerElectronics/SystemModelPowerElectronics.hpp

Line 177 in 12fa7be

if (component->getNodeConnection(j) != neg1_)

GridKit/src/Model/PowerElectronics/SystemModelPowerElectronics.hpp

Line 333 in 12fa7be

static constexpr IdxT neg1_ = static_cast<IdxT>(-1);

but I don't think it is the intended conceptual purpose of using -1.
@reid-g should chime in here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it should just store the largest integer size_t can hold. i went with this as i wasn't totally sure how invasive we should be here (same applies to the other comments). i would much rather

  • either set it to the maximum size_t
  • or adjust the api to provide another way to communicate the same intent (std::optional or something)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is okay as is. The index -1 stands for the ground node. It only needs to be a very distinctive integer.

CC @reid-g

// internal
induct->setExternalConnectionNodes(2, 2);
// add component
Expand All @@ -59,7 +59,7 @@ int main(int /* argc */, char const** /* argv */)
GridKit::VoltageSource<double, size_t>* vsource = new GridKit::VoltageSource<double, size_t>(idoff, vinit);
// Form index to node uid realations
// input
vsource->setExternalConnectionNodes(0, -1);
vsource->setExternalConnectionNodes(0, static_cast<size_t>(-1));
// output
vsource->setExternalConnectionNodes(1, 0);
// internal
Expand Down
2 changes: 1 addition & 1 deletion examples/PowerElectronics/ScaleMicrogrid/ScaleMicrogrid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ int test(index_type Nsize, real_type error_tol, bool debug_output)
dg_ref->setExternalConnectionNodes(1, vdqbus_index[0]);
dg_ref->setExternalConnectionNodes(2, vdqbus_index[0] + 1);
//"grounding" of the difference
dg_ref->setExternalConnectionNodes(3, -1);
dg_ref->setExternalConnectionNodes(3, static_cast<index_type>(-1));
// internal connections
for (index_type i = 0; i < 12; i++)
{
Expand Down
57 changes: 30 additions & 27 deletions src/LinearAlgebra/SparseMatrix/COO_Matrix.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,15 @@ inline std::vector<IdxT> COO_Matrix<ScalarT, IdxT>::getCSRRowData()
{
if (!this->isSorted())
this->sortSparse();
std::vector<IdxT> row_size_vec(this->rows_size_ + 1, 0);
std::vector<IdxT> row_size_vec(static_cast<size_t>(this->rows_size_ + 1), 0);
IdxT counter = 0;
for (IdxT i = 0; i < static_cast<IdxT>(row_size_vec.size() - 1); i++)
{
row_size_vec[i + 1] = row_size_vec[i];
while (counter < static_cast<IdxT>(this->row_indices_.size()) && i == this->row_indices_[counter])
row_size_vec[static_cast<size_t>(i + 1)] = row_size_vec[static_cast<size_t>(i)];
while (counter < static_cast<IdxT>(this->row_indices_.size())
&& i == this->row_indices_[static_cast<size_t>(counter)])
{
row_size_vec[i + 1]++;
row_size_vec[static_cast<size_t>(i + 1)]++;
counter++;
}
}
Expand Down Expand Up @@ -231,33 +232,33 @@ inline void COO_Matrix<ScalarT, IdxT>::setValues(std::vector<IdxT> r, std::vecto
for (IdxT i = 0; i < static_cast<IdxT>(this->row_indices_.size()); i++)
{
// pushback values_ when they are not in current matrix
while (a_iter < static_cast<IdxT>(r.size()) && (r[a_iter] < this->row_indices_[i] || (r[a_iter] == this->row_indices_[i] && c[a_iter] < this->column_indices_[i])))
while (a_iter < static_cast<IdxT>(r.size()) && (r[static_cast<size_t>(a_iter)] < this->row_indices_[static_cast<size_t>(i)] || (r[static_cast<size_t>(a_iter)] == this->row_indices_[static_cast<size_t>(i)] && c[static_cast<size_t>(a_iter)] < this->column_indices_[static_cast<size_t>(i)])))
{
this->row_indices_.push_back(r[a_iter]);
this->column_indices_.push_back(c[a_iter]);
this->values_.push_back(v[a_iter]);
this->checkIncreaseSize(r[a_iter], c[a_iter]);
this->row_indices_.push_back(r[static_cast<size_t>(a_iter)]);
this->column_indices_.push_back(c[static_cast<size_t>(a_iter)]);
this->values_.push_back(v[static_cast<size_t>(a_iter)]);
this->checkIncreaseSize(r[static_cast<size_t>(a_iter)], c[static_cast<size_t>(a_iter)]);
a_iter++;
}
if (a_iter >= static_cast<IdxT>(r.size()))
{
break;
}

if (r[a_iter] == this->row_indices_[i] && c[a_iter] == this->column_indices_[i])
if (r[static_cast<size_t>(a_iter)] == this->row_indices_[static_cast<size_t>(i)] && c[static_cast<size_t>(a_iter)] == this->column_indices_[static_cast<size_t>(i)])
{
this->values_[i] = v[a_iter];
this->values_[static_cast<size_t>(i)] = v[static_cast<size_t>(a_iter)];
a_iter++;
}
}
// push back rest that was not found sorted
for (IdxT i = a_iter; i < static_cast<IdxT>(r.size()); i++)
{
this->row_indices_.push_back(r[i]);
this->column_indices_.push_back(c[i]);
this->values_.push_back(v[i]);
this->row_indices_.push_back(r[static_cast<size_t>(i)]);
this->column_indices_.push_back(c[static_cast<size_t>(i)]);
this->values_.push_back(v[static_cast<size_t>(i)]);

this->checkIncreaseSize(r[i], c[i]);
this->checkIncreaseSize(r[static_cast<size_t>(i)], c[static_cast<size_t>(i)]);
}

this->sorted_ = false;
Expand Down Expand Up @@ -304,34 +305,34 @@ inline void COO_Matrix<ScalarT, IdxT>::axpy(ScalarT alpha, COO_Matrix<ScalarT, I
for (IdxT i = 0; i < static_cast<IdxT>(this->row_indices_.size()); i++)
{
// pushback values when they are not in current matrix
while (a_iter < static_cast<IdxT>(r.size()) && (r[a_iter] < this->row_indices_[i] || (r[a_iter] == this->row_indices_[i] && c[a_iter] < this->column_indices_[i])))
while (a_iter < static_cast<IdxT>(r.size()) && (r[static_cast<size_t>(a_iter)] < this->row_indices_[static_cast<size_t>(i)] || (r[static_cast<size_t>(a_iter)] == this->row_indices_[static_cast<size_t>(i)] && c[static_cast<size_t>(a_iter)] < this->column_indices_[static_cast<size_t>(i)])))
{
this->row_indices_.push_back(r[a_iter]);
this->column_indices_.push_back(c[a_iter]);
this->values_.push_back(alpha * val[a_iter]);
this->row_indices_.push_back(r[static_cast<size_t>(a_iter)]);
this->column_indices_.push_back(c[static_cast<size_t>(a_iter)]);
this->values_.push_back(alpha * val[static_cast<size_t>(a_iter)]);

this->checkIncreaseSize(r[a_iter], c[a_iter]);
this->checkIncreaseSize(r[static_cast<size_t>(a_iter)], c[static_cast<size_t>(a_iter)]);
a_iter++;
}
if (a_iter >= static_cast<IdxT>(r.size()))
{
break;
}

if (r[a_iter] == this->row_indices_[i] && c[a_iter] == this->column_indices_[i])
if (r[static_cast<size_t>(a_iter)] == this->row_indices_[static_cast<size_t>(i)] && c[static_cast<size_t>(a_iter)] == this->column_indices_[static_cast<size_t>(i)])
{
this->values_[i] += alpha * val[a_iter];
this->values_[static_cast<size_t>(i)] += alpha * val[static_cast<size_t>(a_iter)];
a_iter++;
}
}
// push back rest that was not found sorted_
for (IdxT i = a_iter; i < static_cast<IdxT>(r.size()); i++)
{
this->row_indices_.push_back(r[i]);
this->column_indices_.push_back(c[i]);
this->values_.push_back(alpha * val[i]);
this->row_indices_.push_back(r[static_cast<size_t>(i)]);
this->column_indices_.push_back(c[static_cast<size_t>(i)]);
this->values_.push_back(alpha * val[static_cast<size_t>(i)]);

this->checkIncreaseSize(r[i], c[i]);
this->checkIncreaseSize(r[static_cast<size_t>(i)], c[static_cast<size_t>(i)]);
}

this->sorted_ = false;
Expand Down Expand Up @@ -780,7 +781,9 @@ inline void COO_Matrix<ScalarT, IdxT>::sortSparseCOO(std::vector<IdxT>& rows, st
std::sort(std::begin(ordervec),
std::end(ordervec),
[&](int i1, int i2)
{ return (rows[i1] < rows[i2]) || (rows[i1] == rows[i2] && columns[i1] < columns[i2]); });
{ return (rows[static_cast<size_t>(i1)] < rows[static_cast<size_t>(i2)])
|| (rows[static_cast<size_t>(i1)] == rows[static_cast<size_t>(i2)]
&& columns[static_cast<size_t>(i1)] < columns[static_cast<size_t>(i2)]); });

// reorder based of index-sorting. Only swap cost no extra memory.
// @todo see if extra memory creation is fine
Expand Down
Loading
Loading