Try resolving issues discovered with -Wall -Werror #646
Conversation
| @@ -142,7 +142,7 @@ class hiopInterfacePriDecProblem | |||
| const hiopVector& x0, | |||
| const std::string& mem_space); | |||
|
|
|||
| ~RecourseApproxEvaluator(); | |||
| virtual ~RecourseApproxEvaluator(); | |||
There was a problem hiding this comment.
IMHO, this is an important change to ensure that the right destructor is called when delete evaluator is called from hiopAlgPrimalDecomp.cpp.
| @@ -932,7 +925,7 @@ void hiopMatrixSparseCSRSeq::form_transpose_from_symbolic(const hiopMatrixSparse | |||
| } | |||
| //here row_starts_(==w) contains the row starts | |||
| } | |||
| assert(irowptr_[nrows_] = nnz_); | |||
| assert(irowptr_[nrows_] == nnz_); | |||
There was a problem hiding this comment.
This is also another non-trivial change from assignment to equality check.
| @@ -1064,7 +1056,7 @@ void hiopMatrixSparseCSRSeq::form_transpose_from_symbolic(const hiopMatrixSparse | |||
| } | |||
| //here row_starts_(==w) contains the row starts | |||
| } | |||
| assert(irowptr_[nrows_] = nnz_); | |||
| assert(irowptr_[nrows_] == nnz_); | |||
There was a problem hiding this comment.
Assignment to equality check!
|
For those [[maybe_unused]] variables, should I correct them in this PR? Or do you prefer me merging this PR first and then I correct those issues in another PR? @tepperly |
| @@ -106,23 +106,29 @@ DiscretizedFunction::DiscretizedFunction(Ex1Meshing1D* meshing) | |||
| } | |||
|
|
|||
| // u'*v = u'*M*v, where u is 'this' | |||
| double DiscretizedFunction::dotProductWith( const DiscretizedFunction& v_ ) const | |||
| double DiscretizedFunction::dotProductWith( const hiopVector& v_ ) const | |||
There was a problem hiding this comment.
This is to resolve this warning message:
hiop/src/Drivers/Dense/NlpDenseConsEx1.hpp:94:18: error: 'DiscretizedFunction::dotProductWith' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
virtual double dotProductWith( const DiscretizedFunction& v ) const;This stack overflow question addresses this error message.
@nychiang Which ever you prefer. It's up to you. I had to make my changes on a fork of your repository. If you wanted to fix in this PR, I think you'll need to push your changes to my fork of your repo. |
|
Is there something I can do to facilitate the review and merging of this PR? |
it is in the works and on our to-do list for this week. |
|
@tepperly, I just created one PR in your folk, to correct some additional files due to the issue discovered with However, I wonder if you could work on this PR in the LLNL folk, i.e., move I am not sure why all the CI tests do not work while your PR is initialized from an external folk. |
|
@tepperly With @cameronrutherford's help, the PNNL CI is working now. Once you file this PR within LLNL domain, we can get feedbacks from all the CI tests. |
I think you need to give user @tepperly access to push to your repo. I forked because I can't push to a branch on $ git push origin feature/headers-wall-werror-proof
ERROR: Permission to LLNL/hiop.git denied to tepperly.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists. |
|
@tepperly I just added you into the repository. You may receive an email for confirmation. |
|
https://gitlab.pnnl.gov/exasgd/frameworks/exago/-/merge_requests/488 - recently did this in ExaGO FWIW so should be reasonably easy to add this to HiOp pipelines as well if you want |
I tried switching to C++ 17 and turning on
-Wall -Werrorduring compile. This found a variety of issues.It's hard for me as an outside to know when unused variables can be deleted versus just marking them as ``[[maybe_unused]]`. The key here is whether calculating the variable value has important side effects.
The main changes here involve:
intandsize_t(unsigned long) in conditionals[[maybe_unused]]variables that are never used=was used instead of==inside an assertvirtual