[WIP] Warnings #480
[WIP] Warnings #480
Conversation
|
The Line 132 in b820933 This warning can be removed by initializing it in the constructor: or, by just removing the padding. At least on my system (gcc 11.4.0) Which do you prefer? |
|
You can define copy/move constructors/assignment operators that skip copying the padding: tree_node(const tree_node& other):
childs(other.childs),
sum_results(other.sum_results),
num_visits(other.num_visits),
var(other.var),
finished(other.finished) {}
tree_node(const tree_node&& other) noexcept :
childs(std::move(other.childs)),
sum_results(other.sum_results),
num_visits(other.num_visits),
var(other.var),
finished(other.finished) {}
tree_node& operator=(const tree_node& other) {
if (this != &other) {
childs = other.childs;
sum_results = other.sum_results;
num_visits = other.num_visits;
var = other.var;
finished = other.finished;
}
return *this;
}
tree_node& operator=(tree_node&& other) noexcept {
if (this != &other) {
childs = std::move(other.childs);
sum_results = other.sum_results;
num_visits = other.num_visits;
var = other.var;
finished = other.finished;
}
} |
|
Ah, right, now I understand where the warning comes from. You need a On my system at least, the build is now clean, except the still-disabled |
|
I think we don't want to use (I wonder, if FORM 5 could actually be the time to drop 32bit support? #422 #426 ) The current fix for 12ce0a5 fails on macOS because it needs to be built specifying c++11. We could do that, or change the fix. |
|
For the "C++11 extension" errors, we can test if the compiler accepts |
|
That's fine as long as macOS will always support those flags. I don't have a mac to test this. |
|
docker run -it --rm ubuntu:22.04apt update
apt-get install -y automake g++-12 git libgmp-dev libmpfr-dev make ruby zlib1g-dev
git clone https://github.com/jodavies/form.git
cd form
git checkout 56f5146d7d43e3d9dc8faddcae42621639274e2e
autoreconf -if
CC=gcc-12 CXX=g++-12 ./configure
make |
|
Yes, I will remove that again and clean up this patch series. For 33f7021, should we remove c++11 features, or add it to the build? |
|
Perhaps, we may need a consensus on the minimal C/C++ requirements, like C99/C++11. The workshop would be a good opportunity for it. Even if we do not exclude C++98/C++03, we may use the |
|
I cleaned this up somewhat. For now I just removed the cpp11 code, it compiles without warnings without the move constructor anyway. Hopefully macOS is happy this time... |
|
Is this PR still in progress? Or is it ready to merge? |
|
At the moment the build is not completely warning free, depending on which compiler or version you use. I don't think that is necessarily the goal though. Primarily I wanted to clean up things that potentially lead to bugs, such as uninitialized variables and, particularly, misleading indentation (sorting that one out took me several iterations to do correctly, in the SKIPBRA macros -- precisely the reason why it should be resolved, I suppose). I would say this can be merged, and additional warnings can be cleaned up in the future as and when it seems necessary. |
|
@tueda : would you want these commits squashed or as they are now: one per warning? |
|
I think both options are fine; keeping them as a bunch of separate commits is reasonable for this case. It is just I haven't tried to merge this PR because there are still warnings. Merging it would be OK. One possible improvement could be: |
|
Which compiler do you see this warning with? |
|
I meant the warnings with 32-bit containers in CI. With GCC 14 on Ubuntu 64-bit, there are no warnings. (Warnings for LLP64 are compatibility bugs, so that's another story.) |
|
The 32bit build has a number of warnings due to With gcc-12 (both 32 and 64 bits) I also actually get a I don't think the code is really doing anything wrong here, probably we can ignore this one: Lines 1442 to 1443 in 83e3d41 |
|
With the current commits, 64bit form and tform (and vorm and tvorm) build with no warnings on Ubuntu 20.04, 24.04, 24.10. Parform is OK on 20.04 (mpich), 24.04 (openmpi), but not on 24.10 (mpich), there are |
|
This looks fine to merge (after rearranging the commits and commit messages). In some cases warnings might still persist or reappear in the future, but it's certainly a step toward better code quality. |
|
It took me a little while to work out that the brackets are required on the |
Yes, this is very tricky and related to a reason why C++ers prefer using |
Clean up maybe-uninitialized warnings. In optimize.cc: tree_node contains alignment padding, which was "used" uninitialized. Add copy constructors and assignment ops which ignore the padding, from @tueda.
Add braces to resolve cases of misleading-indentation
Rename "alignof" macro to "form_alignof" to remove clang warning. We can also then remove the definition of _ALLOW_KEYWORD_MACROS for MSVC.
These bindings are not used. Define the pre-proc variable OMPI_SKIP_MPICXX which skips their definition.
Add braces around logical not used on LHS of comparison
Use FORM's DUMMYUSE macro to suppress (valid) warning for an unused parameter.
Adjust NCOPY macro to avoid stringop-overflow warning.
|
I made the commit messages a bit more uniform, and merged the two for |
I'm trying to clean up the compilation warnings. Remaining on my system, is a
uninitializedand akeyword-macro(with clang 14).