vermaseren / form

The FORM project for symbolic manipulation of very big expressions
GNU General Public License v3.0
1.15k stars 136 forks source link

Too many gcc warnings #298

Closed tueda closed 5 years ago

tueda commented 5 years ago

At some point we need to think about warnings by gcc7/8, which claims FORM source is misleading etc. (See also which gcc versions many people may use.)

Options:

  1. keep ignoring them, though we may miss some important warning slipping in them,
  2. suppress the warnings by compiler options and keep FORM source being misleading, or
  3. demystify FORM source.
vermaseren commented 5 years ago

In principle option 3 is best, although if you talk about hundreds of messages that may be a lot of work. I do like to keep the code ‘clean’.

On 26 Oct 2018, at 22:47, Takahiro Ueda notifications@github.com wrote:

At some point we need to think about warnings by gcc7/8 https://travis-ci.org/tueda/form-build-test/jobs/445141800, which claims FORM source is misleading etc. (See also which gcc versions https://repology.org/metapackage/gcc/versions many people may use.)

Options:

keep ignoring them, though we may miss some important warning slipping in them, suppress the warnings by compiler options and keep FORM source being misleading, or demystify FORM source. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/298, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEl3bD66JpmWcxDNWDNvUIEs3jZIgks5uoxJigaJpZM4X8RQE.

tueda commented 5 years ago

If I search warning: in that page, there are 103 matches. This number is for FORM + TFORM, so about 50 warnings to be cleaned :-)

vermaseren commented 5 years ago

Sounds doable. Probably mostly the same construction. How do I get the compiler that does this either on my Mac or on one of the Nikhef computers?

On 26 Oct 2018, at 22:56, Takahiro Ueda notifications@github.com wrote:

If I search warning: in that page, there are 103 matches. This number is for FORM + TFORM, so about 50 warnings to be cleaned :-)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/298#issuecomment-433416292, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxElS0KUDyoqBDfAJeIp9db1KhMaLoks5uoxR9gaJpZM4X8RQE.

tueda commented 5 years ago

I guess you still have Homebrew on you mac, and it has now gcc 8.2.0.

vermaseren commented 5 years ago

OK, It did indeed install and I managed to remove those warnings. Most were of the type that it did not like while ( *s ) s++; s++; but wanted the second s++; on a new line. At the moment it translates again (on my MacBook) without warnings.

On 26 Oct 2018, at 23:37, Takahiro Ueda notifications@github.com wrote:

I guess you still have Homebrew on you mac, and it has now gcc 8.2.0 https://github.com/Homebrew/homebrew-core/blob/master/Formula/gcc.rb.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/298#issuecomment-433429838, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEj_kfMugDcJQnswvy8Q0klR3bRyFks5uox4VgaJpZM4X8RQE.

tueda commented 5 years ago

Nice! Don't you see any warnings on your Mac? Still we have 55 matches of warnings: in this log, like Wcast-function-type or Wimplicit-fallthrough. Or you have just removed only Wmisleading-indentation?

vermaseren commented 5 years ago

Those were all the warnings my system gave. Maybe there are some extra compiler settings that give those extra warnings. Once I can reproduce them here, I can attack them.

On 27 Oct 2018, at 18:25, Takahiro Ueda notifications@github.com wrote:

Nice! Don't you see any warnings on your Mac? Still we have 55 matches of warnings: in this log https://travis-ci.org/tueda/form-build-test/jobs/445141800, like Wcast-function-type or Wimplicit-fallthrough. Or you have just removed only Wmisleading-indentation?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/298#issuecomment-433605277, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEgiQaFMKYUz996Oj-7UErkSApnacks5upCZwgaJpZM4X8RQE.

tueda commented 5 years ago

Maybe you don't use the -Wextra option, which enables some extra warning options. With that option, you will see many warnings, though we can argue if it is more harm than good and not needed.

vermaseren commented 5 years ago

I have a problem here. All those fallthroughs are intended. It is part of the language. Why do they issue warnings about it? I am not going to double lots of code just because they think that every case needs a break. What am I supposed to do then?

On 27 Oct 2018, at 18:47, Takahiro Ueda notifications@github.com wrote:

Maybe you don't use the -Wextra option, which enables some extra warning options. With that option, you will see many warnings, though we can argue if it is more harm than good and not needed.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/298#issuecomment-433606614, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEuEl52-fCSy78w5AnTSC0PLoSFkOks5upCuWgaJpZM4X8RQE.

tueda commented 5 years ago

Why do they issue warnings about it?

Because people consider it as a design defect in C and implicit use of it is most likely a bug by the programmer. See this page for why and how to fix it. C++17 has the standard way to declare an fall-through is intended. Probably C2x will also do so.

But maybe we can switch off "implicit-fallthrough" warnings for now.

By the way, if you put the -pedantic option you will get more warnings because FORM is not standard compliant.

vermaseren commented 5 years ago

I will commit/push the -Wextra problems. But the -Wpedantic will have to wait. I did have a look at it and it concentrates purely around pointers to functions not being allowed to be cast to ther pointers. A nice fraction can be intercepted I believe by changing a few declarations in the structs, but I better think this over first.

On 27 Oct 2018, at 19:17, Takahiro Ueda notifications@github.com wrote:

Why do they issue warnings about it?

Because people consider it as a design defect in C and implicit use of it is most likely a bug by the programmer. See this page https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/ for why and how to fix it. C++17 has the standard way to declare an fall-through is intended. Probably C2x will also do so.

But maybe we can switch off "implicit-fallthrough" warnings for now.

By the way, if you put the -pedantic option you will get more warnings because FORM is not standard compliant.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/298#issuecomment-433608490, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEuBaGPMik4hx191cEmLyK2pIh_HSks5upDLTgaJpZM4X8RQE.

vermaseren commented 5 years ago

One problem. I checked, before committing my changes, that it still works with the old compiler. I added everywhere
attribute ((fallthrough)); and now the old compiler complains that the declaration does not declare anything. [-Wmissing-declarations]. Hence: that is not a good fix of those guys. Suggestions?

On 27 Oct 2018, at 19:58, Jos Vermaseren t68@nikhef.nl wrote:

I will commit/push the -Wextra problems. But the -Wpedantic will have to wait. I did have a look at it and it concentrates purely around pointers to functions not being allowed to be cast to ther pointers. A nice fraction can be intercepted I believe by changing a few declarations in the structs, but I better think this over first.

On 27 Oct 2018, at 19:17, Takahiro Ueda <notifications@github.com mailto:notifications@github.com> wrote:

Why do they issue warnings about it?

Because people consider it as a design defect in C and implicit use of it is most likely a bug by the programmer. See this page https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/ for why and how to fix it. C++17 has the standard way to declare an fall-through is intended. Probably C2x will also do so.

But maybe we can switch off "implicit-fallthrough" warnings for now.

By the way, if you put the -pedantic option you will get more warnings because FORM is not standard compliant.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/298#issuecomment-433608490, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEuBaGPMik4hx191cEmLyK2pIh_HSks5upDLTgaJpZM4X8RQE.

tueda commented 5 years ago

Maybe just writing /* fall through */ or /* FALLTHROUGH */ would be enough? If it turns out to be not so good, later we can replace such phrases with an attribute or something.

vermaseren commented 5 years ago

Yes, I just found that one with google. I have already tried it in one file and it seems to work. It turns out that the new compiler does try to see whether the fall through was intended by looking for commentary. I consider this a bit dirty I must say. I’ll fix it up and commit once I am happy with it. The pedantic is not for now.

On 27 Oct 2018, at 20:38, Takahiro Ueda notifications@github.com wrote:

Maybe just writing / fall through / https://github.com/torvalds/linux/search?q=fall+through&unscoped_q=fall+through&type=Code or / FALLTHROUGH / https://github.com/torvalds/linux/search?q=FALLTHROUGH&unscoped_q=FALLTHROUGH&type=Code would be enough? If it turns out to be not so good, later we can replace such phrases with an attribute or something.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/298#issuecomment-433613163, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEskuFxlefpM6_Bn9pjBnIx8NA-LDks5upEW7gaJpZM4X8RQE.

tueda commented 5 years ago

I guess most of the warnings are removed (except -Wmaybe-uninitialized of gcc7). So I close this issue.