Skip to content

Use LastIndexOf in MSBuild for a single char#60450

Merged
marek-safar merged 1 commit into
dotnet:mainfrom
KirillOsenkov:dev/kirillo/lastIndexOfAny
Oct 30, 2021
Merged

Use LastIndexOf in MSBuild for a single char#60450
marek-safar merged 1 commit into
dotnet:mainfrom
KirillOsenkov:dev/kirillo/lastIndexOfAny

Conversation

@KirillOsenkov
Copy link
Copy Markdown
Member

No need to call LastIndexOfAny for a single char. MSBuild doesn't (yet) have a shortcut for evaluating LastIndexOfAny, so it causes first-chance MissingMethodExceptions in MSBuild evaluator.

Also strictly speaking passing a string to LastIndexOfAny doesn't make sense since it accepts a char array.

No need to call LastIndexOfAny for a single char. MSBuild doesn't (yet) have a shortcut for evaluating LastIndexOfAny, so it causes first-chance MissingMethodExceptions in MSBuild evaluator.

Also strictly speaking passing a string to LastIndexOfAny doesn't make sense since it accepts a char array.
@KirillOsenkov KirillOsenkov requested a review from ericstj October 15, 2021 00:53
@ghost
Copy link
Copy Markdown

ghost commented Oct 15, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@KirillOsenkov
Copy link
Copy Markdown
Member Author

See related: dotnet/msbuild#6963

Copy link
Copy Markdown
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

@am11
Copy link
Copy Markdown
Member

am11 commented Oct 15, 2021

It was changed from LastIndexOf to LastIndexOfAny in #31701, then this code moved around.

MSBuild doesn't (yet) have a shortcut for evaluating LastIndexOfAny, so it causes first-chance MissingMethodExceptions in MSBuild evaluator.

is it non-fatal? I asked because it seems to be working so far.

@danmoseley
Copy link
Copy Markdown
Member

Yes the MSBuild "binder" is a bunch of reflection heuristics, some of which involve first chance exceptions, preceded by a hard coded list of common and builtin methods (like LastIndexOf)

There wasn't a convenient binder around at the time so we wrote this custom one.

Copy link
Copy Markdown
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Thank you, @KirillOsenkov

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Oct 15, 2021

I found the origin, dotnet/coreclr@9497910#diff-5076575030d3845a817fab2cd48d02742f0f90f479dcf582057a869f003f5502R11

That made a bit more sense. IIRC that also caused a first chance exception.

@marek-safar marek-safar merged commit 8bba6fb into dotnet:main Oct 30, 2021
@KirillOsenkov KirillOsenkov deleted the dev/kirillo/lastIndexOfAny branch October 30, 2021 17:20
@ghost ghost locked as resolved and limited conversation to collaborators Nov 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants