I ran our Github actions without the ignore statements. Surprisingly, all of this never caused the Github action to fail. A while ago, we removed flags "-Werror" and the windows equivalent "/WX" from the CMake files. For the Ubuntu build, we then explicitly export CXXFLAGS="-Werror" in the Github workflow, but for Windows we don't. I suggest we fix this.
I removed all ignore statements for warnings that never triggered. The remaining ignores fall in two categories: lossy conversions and shadowing.
The lossy conversions (C4244, C4267), I'd say are fine to ignore. In particular C4267 is about size_t to int which we do a lot. The other conversions are
* from __int64 (integer literal, I think) to int, relaxation_heuristic::OpID, and relaxation_heuristic::PropID;
* from int to _Ty (template arg?), char, and short;
* and from double and size_t to int.
The shadowing looks more like something we might want to fix in the code. Here are some examples for all types of warnings:
* C4458: declaration of 'num_variables' hides class member
class State {
int num_variables;
void State::unpack() const {
int num_variables = size();
// ...
}
}
In this particular case, size() just returns the member num_variables. This looks completely unnecessary and we should fix it. There are other cases that are less obvious. I'll post them in a separate message.
* C4456: declaration hides previous local variable
for (int i = 0; i < n1; ++i) {
int j = 0;
// do something with j
for (int j = 0; j < n2; ++j) {
// ...
}
}
Some of these look like potential problems, but we also heavily use this in the parser with trace blocks that all have the same name. We could give them different names but calling them all block emphasizes that their identity is not important and simplifies the code in my opinion.
{
Traceblock block("list");
parse_open_bracket();
for (int i = 0; i < n; ++i) {
Traceblock block("element " + i);
parse_element();
}
parse_closing_bracket();
}
Concrete cases are here:
https://github.com/aibasel/downward/blob/main/src/search/heuristics/domain_transition_graph.cc#L96 (origin)
https://github.com/aibasel/downward/blob/main/src/search/landmarks/landmark_factory_h_m.cc#L370 (pm_fluent)
https://github.com/aibasel/downward/blob/main/src/search/parser/abstract_syntax_tree.cc#L276 (block)
* C4459: declaration hides global declaration
void f(const equivalence_relation::EquivalenceRelation &relation) {
// ...
}
Here 'relation' shadows 'std::relation'. We also get this for variables named 'predicate' and 'equivalence_relation'. All of these are not immediate problems for us, I'd say.
|