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

remove usages of DefinedTypes #2915

Closed

Conversation

SimonCropp
Copy link
Contributor

DefinedTypes is essentially an inefficient wrapper around GetTypes().

   public virtual IEnumerable<TypeInfo> DefinedTypes
        {
            [RequiresUnreferencedCode("Types might be removed")]
            get
            {
                Type[] types = GetTypes();
                TypeInfo[] typeinfos = new TypeInfo[types.Length];
                for (int i = 0; i < types.Length; i++)
                {
                    TypeInfo typeinfo = types[i].GetTypeInfo();
                    if (typeinfo == null)
                        throw new NotSupportedException(SR.Format(SR.NotSupported_NoTypeInfo, types[i].FullName));

                    typeinfos[i] = typeinfo;
                }
                return typeinfos;
            }
        }

better to just use GetTypes() directly and not bother messing with TypeInfo

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Look like some more code needs to be changed. Also, I am a bit nervous to merge this change right now. @nohwnd discovered that that reflection tests are relatively bad or broken in #2839 so he is working on improving the tests before continuing his work.

@Evangelink
Copy link
Member

The PR is now touching more files than expected but I was getting a lot of IDE warnings popping (not sure why now).

@Evangelink Evangelink enabled auto-merge (squash) May 30, 2024 09:32
@SimonCropp
Copy link
Contributor Author

@Evangelink shall we close, and i till try for some more narrowly targeted PRs?

@Evangelink
Copy link
Member

Linux errors are just a missing rebase, I broke main without noticing. It's fixed now.

For windows, and I can repro locally, it seems that the tests are getting stuck. Not sure why we don't end up with the dumps created (cc @MarcoRossignoli)

@Evangelink
Copy link
Member

Rebasing, I have managed to run all tests locally so it should be green in CI now.

@Evangelink
Copy link
Member

Closing this one.

@Evangelink Evangelink closed this Jun 17, 2024
auto-merge was automatically disabled June 17, 2024 09:24

Pull request was closed

@SimonCropp SimonCropp deleted the remove-usages-of-DefinedTypes branch June 17, 2024 10:45
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.

2 participants