Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NativeAOT-LLVM] Fix most warnings for the HelloWasm runtime test #2168

Merged
merged 4 commits into from
Jan 16, 2023

Conversation

SingleAccretion
Copy link

Only two, related to the missing dotnet_browser_entropy symbol, remain.

Contributes to #2165.

There are two throw helpers used for tentative methods: "ThrowBodyRemoved"
and "ThrowInstanceBodyRemoved". We were always using the first one.
We import them manually from a shared location.

Usually this is not a problem because batch build (via the build script)
does its own restore thing, but it is a problem in case the project is
restored on its own, such as when it is built with "dotnet build".
@SingleAccretion SingleAccretion marked this pull request as ready for review January 13, 2023 22:16
@SingleAccretion
Copy link
Author

@dotnet/nativeaot-llvm

Copy link
Contributor

@yowl yowl left a comment

Choose a reason for hiding this comment

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

LGTM, modulo if @jkotas has any comment on the protected/public comment

@@ -35,11 +35,11 @@ public TentativeInstanceMethodNode(IMethodBodyNode methodNode)

public override bool HasConditionalStaticDependencies => true;

protected override ISymbolNode GetTarget(NodeFactory factory)
public override IMethodNode GetTarget(NodeFactory factory)
Copy link
Contributor

Choose a reason for hiding this comment

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

This visibility change seems harmless to me, but maybe @jkotas you'd like to cast an eye on it?

Copy link
Member

Choose a reason for hiding this comment

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

It looks good to me. @MichalStrehovsky Any concerns?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, looks good. All AssemblyStubNode descendants need some special treatment because on WASM we're not able to generate byte buffers with extra code in it (which is what we do elsewhere).

@SingleAccretion SingleAccretion changed the title Fix most warnings for the HelloWasm runtime test [NativeAOT-LLVM] Fix most warnings for the HelloWasm runtime test Jan 15, 2023
@jkotas jkotas merged commit 07fd0a3 into dotnet:feature/NativeAOT-LLVM Jan 16, 2023
@SingleAccretion SingleAccretion deleted the Warnings branch January 17, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants