Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/3.1] Fix corefx to build on clang 10 #42900

Merged
merged 1 commit into from
May 13, 2020

Conversation

omajid
Copy link
Member

@omajid omajid commented Apr 3, 2020

Clang 10 adds/enables new warnings, some of which is affecting the corefx code.

Clang 10 has added -Walloca to warn about uses of alloca(). Clang 10 has also added -Wimplicit-int-float-conversion. This commit disables those warnings where applicable.

This is a backport of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.

@omajid
Copy link
Member Author

omajid commented Apr 6, 2020

cc @jkotas @lpereira @stephentoub

Copy link

@lpereira lpereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with suggestions

@omajid
Copy link
Member Author

omajid commented Apr 6, 2020

@lpereira

LGTM with suggestions

Should I make these suggestions in runtime too? This same PR was applied (and merged) into runtime already: dotnet/runtime#33734

@jkotas
Copy link
Member

jkotas commented Apr 6, 2020

Is this just fixing warnings? For servicing, we should disable the warnings instead of touching the code.

@jkotas
Copy link
Member

jkotas commented Apr 6, 2020

cc @danmosemsft

@danmoseley
Copy link
Member

If we can dial this back to disabling warnings, I am happy to seek approval to merge it.

omajid added a commit to omajid/dotnet-runtime that referenced this pull request Apr 14, 2020
@omajid omajid force-pushed the clang10 branch 2 times, most recently from 4d59310 to 827913e Compare April 14, 2020 22:11
stephentoub pushed a commit to dotnet/runtime that referenced this pull request Apr 15, 2020
Clang 10 adds/enables new warnings, some of which is affecting the
corefx code. Disable them.

This is a "backport" of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.
@jkotas
Copy link
Member

jkotas commented Apr 15, 2020

@danmosemsft This is dialed back to disabling warnings now. Could you please get it approved?

@danmoseley danmoseley added the Servicing-consider Issue for next servicing release review label Apr 15, 2020
@danmoseley
Copy link
Member

Yup - @omajid can you share why you're working to enable build with clang 10 ? Does RedHat plans to build its 3.1 servicing releases using clang 10?

@omajid
Copy link
Member Author

omajid commented Apr 16, 2020

Does RedHat plans to build its 3.1 servicing releases using clang 10?

I am not aware of any concrete plans for RHEL.

can you share why you're working to enable build with clang 10 ?

My main motivation is to build .NET Core in Fedora (>= 32) where clang is at version 10. We are currently carrying a local patch for that: https://src.fedoraproject.org/fork/crummel/rpms/dotnet3.1/blob/master/f/corefx-42900-clang-10.patch. I prefer to avoid carrying these patches out of band where possible.

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 16, 2020
@leecow leecow added this to the 3.1.5 milestone Apr 16, 2020
@danmoseley
Copy link
Member

@Anipik will merge when branch open

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants