Page MenuHomePhabricator

[Diagnostics] Support -Wtype-limits for GCC compatibility
ClosedPublic

Authored by xbolva00 on Mar 1 2019, 11:10 AM.

Details

Diff Detail

Repository
rC Clang

Event Timeline

xbolva00 created this revision.Mar 1 2019, 11:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2019, 11:10 AM
xbolva00 edited the summary of this revision. (Show Details)Mar 1 2019, 11:11 AM
xbolva00 edited the summary of this revision. (Show Details)
lebedev.ri requested changes to this revision.Mar 1 2019, 12:03 PM
lebedev.ri added a subscriber: lebedev.ri.
clang/include/clang/Basic/DiagnosticGroups.td
784

-Wtautological-constant-in-range-compare is quite intentionally not in -Wextra.

This revision now requires changes to proceed.Mar 1 2019, 12:03 PM
xbolva00 updated this revision to Diff 188957.Mar 1 2019, 12:10 PM

Remove from -Wextra

lebedev.ri resigned from this revision.Mar 1 2019, 12:20 PM
lebedev.ri added a reviewer: thakis.
lebedev.ri added inline comments.
clang/include/clang/Basic/DiagnosticGroups.td
485

Is gcc's -Wtype-limits *just* -Wtautological-constant-in-range-compare, or is something else should be there?

xbolva00 marked an inline comment as done.EditedMar 1 2019, 12:36 PM
This comment has been deleted.
clang/include/clang/Basic/DiagnosticGroups.td
485

-Wtautological-constant-in-range-compare is enough to diagnose 99% cases from:

https://github.com/gcc-mirror/gcc/blob/41d6b10e96a1de98e90a7c0378437c3255814b16/gcc/testsuite/g%2B%2B.dg/warn/Wtype-limits.C

One missed case is:
int test (int x)
{

if ((long long)x <= 0x123456789ABCLL)
  return 1;
else 
  return 0;

}

Tried -Weverything, no help. We miss this case. I will open PR for that.

But anyway, this missed case should not block this patch.

lebedev.ri added inline comments.Mar 1 2019, 12:44 PM
clang/include/clang/Basic/DiagnosticGroups.td
485

-Wtautological-constant-in-range-compare is enough to diagnose 99% cases from:

That doesn't really answer the question.
I'm sure that *adding* -Wtautological-constant-in-range-compare helps.
But what about the other way around?
Does passing -Wno-type-limits silence *all* the expected diags in that file?

xbolva00 marked an inline comment as done.Mar 1 2019, 12:52 PM
xbolva00 added inline comments.
clang/include/clang/Basic/DiagnosticGroups.td
485

Yes, silences it correctly.

I worry a little bit about our -Wtype-limits getting out of sync from GCC's due to making it a synonym for -Wtautological-constant-in-range-compare, but I'm also at a loss for why these two should ever have different functionality.

I think I'm okay with this, but I'd like @rsmith to weigh in as well.

clang/include/clang/Basic/DiagnosticGroups.td
485
xbolva00 abandoned this revision.Apr 29 2019, 2:08 PM

xbolva00 abandoned this revision.

Do you not want to pursue this any more?

This seems reasonable to me, and is in line with other cases where we have diagnostic flags as aliases to GCC's similar-but-not-quite-the-same flags (eg, GCC's -Wnoexcept-type doesn't fire in quite the same set of cases as clang's -Wc++17-compat-mangling, but we alias them; likewise for GCC -Wsequence-point versus Clang -Wunsequenced).

xbolva00 reclaimed this revision.Apr 29 2019, 2:30 PM

Ok, reclaiming patch.

Thanks for the review! If the patch is fine, please approve it.

rsmith accepted this revision.Apr 29 2019, 2:45 PM

Thanks for the review! If the patch is fine, please approve it.

Sure thing! (Phab doesn't permit approving a patch in "abandoned" state.)

This revision is now accepted and ready to land.Apr 29 2019, 2:45 PM
This revision was automatically updated to reflect the committed changes.