Comment 64 for bug 1657256
Revision history for this message
| Christian Ehrhardt (paelzer) wrote | #64 |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
I was asked to help with the review and on a first glimpse agree with Robie c#62.
TL;DR:
1. munching various changes together makes it hard to review
2. several changes are not clear why they were added to the debdiff
2. We are in Final Freeze already, so SRU Rules apply -> lets fix the FTBFS and drop all not needed changes which make the change easier to grasp, better to review and safer to apply.
If you could adapt a git based workflow for complex uploads that would help review and changes as well.
I'll rework the patch in a way I assume it might work.
Then I ask Jorge to review if this is still ok with him.
I can recreate the build errors on the last upload.
It matches those in [1].
I checked Robies comments above and referencing his changes I did:
1. really not needed atm - dropping
2. really not needed atm - dropping
3. the effect of this depends on defines like IB_STRONG_
For Intel this is still a no-op (as the other defines), just now a defined
instead of an undefined no-op define.
=> keep this change
4. Yes this is like "compiler please check nothing" which I think is too much
The code actually already takes a very detailed list of ignores to run with -Werror despite
some warnings. If you look at [1] you see e.g. -Wno-error=
-Wno-
a few.
Dropping -Werror in general and even setting fpermissive [2] is much more than needed
This is new in gcc7 [3] and that is what we look for.
=> use -Wno-error=
=> tests showed a more cases that are needed like format-overflow, ... (added as needed)
=> See below for details on permissive
5. nice cleanup but not needed
=> drop
6. This might be to make the defines available earlier?
I tested to build without and it was fine on x86.
7. I'll have to rewrite the changelog anyway, so that will be cleaned
This exercise (other than review) of rewriting the fix was mostly disabling the new errors, but one was special. It now better detects ISO standard violations.
That causes:
"error: ISO C++ forbids comparison between pointer and integer [-fpermissive]"#
Now one might think, lets add that because the doc says:
-fpermissive
Downgrade some diagnostics about nonconformant code from errors
to warnings. Thus, using -fpermissive allows some nonconforming
code to compile.
But that makes something that is an error a warning, which still is an error with -Werror.
You can't further downgrade this with -Wno-... [4]
Instead these errors need to be fixed in code - otherwise we would have to drop -Werrror which I would not like.
Test Program:
#include <iostream>
using namespace std;
int main()
{
const char *str = "Foo";
if (str != '\0')
{
cout << "Hello, World!";
}
return 0;
}
I fixed it with a in-source fix "fix-gcc7-
I'd ask you Jorge, to please discuss to upstream this.
After quite some iterations I was successful with that on a local build.
Switching to cross arch build on Launchpad to be entirely sure - this is still building, so further respins might be needed if something shows up there.
@Jorge - If a test would fast and the build succeeds, please consider testing the ppa at [5].
I upload this for review (Jorge if you want, Rbasak for sure but it should be easier to look at now) and sponsoring (Rbasak).
Please look at the changes suggested in [6], I wasn't sure if you want a normal MP after so much review was on the debdiff. But if you want there would be one in [7] - that would give us commentary functions of LP.
[1]: https:/
[2]: https:/
[3]: https:/
[4]: https:/
[5]: https:/
[6]: https:/
[7]: https:/