Page MenuHomePhabricator

[X86] Fix implicit sign conversion warnings in X86 headers.
ClosedPublic

Authored by pgousseau on Apr 3 2020, 6:51 AM.

Diff Detail

Event Timeline

pgousseau created this revision.Apr 3 2020, 6:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 6:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
RKSimon added a subscriber: RKSimon.

Rename test file to x86-header-warnings.c or something similarly specific?

clang/test/Headers/warn-cleanup.c
10 ↗(On Diff #254784)

Use x8intrin.h directly so its easier to expand in the future?

pgousseau updated this revision to Diff 254827.Apr 3 2020, 8:46 AM

Renaming test and including x86intrin.h instead.

@craig.topper Any comments? Hopefully we can add extra tests as time goes on (for instance I'm intending to add Wdocumentation test coverage for PR35949).

clang/test/Headers/x86-header-warnings.c
12

Add a LABEL check as well

Seems fine to me.

RKSimon accepted this revision.Apr 6 2020, 11:41 AM

LGTM with the minor addition an extra CHECK-LABEL

This revision is now accepted and ready to land.Apr 6 2020, 11:41 AM
pgousseau updated this revision to Diff 255626.Apr 7 2020, 3:21 AM

Added label

thakis added inline comments.
clang/test/Headers/x86-header-warnings.c
4

Does Wsystem-headers add too much noise?

Closed by commit rG08fab9ebecf7: [X86] Fix implicit sign conversion warnings in X86 headers. (authored by Pierre Gousseau <pierre.gousseau@sony.com>). · Explain WhyApr 7 2020, 3:45 AM
This revision was automatically updated to reflect the committed changes.
pgousseau marked 2 inline comments as done.Apr 7 2020, 4:07 AM
pgousseau added inline comments.
clang/test/Headers/x86-header-warnings.c
4

Thanks for the suggestion, I did not thought about adding Wsystem-headers, I tested Wsystem-headers now though and cant seem to pick up on that particular issue without the preprocessing step.

12

Adding label sounds good, went ahead and commited without checking the test was not failing anymore, sorry will revert.

pgousseau reopened this revision.Apr 7 2020, 6:43 AM

Reopening to fix false negative test.

This revision is now accepted and ready to land.Apr 7 2020, 6:43 AM
pgousseau updated this revision to Diff 255666.Apr 7 2020, 6:45 AM

Fix false negative test introduce while attempting to add label check.

It looks like you're not actually interested in the compiled output, but just whether warnings occurred; in that case you'd be better off with -verify -fsyntax-only and // expected-no-diagnostics.

It looks like you're not actually interested in the compiled output, but just whether warnings occurred; in that case you'd be better off with -verify -fsyntax-only and // expected-no-diagnostics.

Sounds good! Committed at https://github.com/llvm/llvm-project/commit/937e63b8d5e961c2a7da25558bbcdd5388182b67
Thanks!

This revision was automatically updated to reflect the committed changes.