Skip to content

Delete unused native code in src/native #67087

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

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Mar 24, 2022

No description provided.

@ghost
Copy link

ghost commented Mar 24, 2022

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.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 24, 2022
@ghost
Copy link

ghost commented Mar 24, 2022

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: am11
Assignees: -
Labels:

area-Infrastructure, community-contribution

Milestone: -

@am11 am11 marked this pull request as ready for review March 24, 2022 09:46
@akoeplinger
Copy link
Member

I'm fine with the PAL changes but not sure about the corehost/eventpipe ones so I'll let the others chime in.

@@ -15,12 +15,10 @@ struct version_t
int get_major() const { return m_major; }
int get_minor() const { return m_minor; }
int get_build() const { return m_build; }
int get_revision() const { return m_revision; }
Copy link
Member

Choose a reason for hiding this comment

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

/cc @elinor-fung @vitek-karas for corehost sub-dir.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

corehost looks good to me - minor suggestion for possibly deleting some more code.


void set_major(int m) { m_major = m; }
void set_minor(int m) { m_minor = m; }
void set_build(int m) { m_build = m; }
void set_revision(int m) { m_revision = m; }
Copy link
Member

Choose a reason for hiding this comment

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

I think we can actually remove all the set_* in version_t and fx_ver_t.

Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

For the EventPipe changes, most of these are utility/library functions and some where original part of the ported C++ library and have been used on and off in the codebase over years, I still see potential value in some of them and they are all static inline, so might end up as unused static function in compile unit and removed by linker. No problem removing them, but I also don't see the problem having them around, or does it trigger some unused code warnings?

@am11
Copy link
Member Author

am11 commented Mar 25, 2022

We usually delete the code which is unused. We can always bring it back if needed (which seldom happens).

@lateralusX
Copy link
Member

We usually delete the code which is unused. We can always bring it back if needed (which seldom happens).

If this is inline with some best practices I won't object to the changes. Once the code gets removed I believe it will be hard for people not directly involved in the removal to start look for an old removed function that they don't know exists, so I guess it will result in rewriting the code, but since the above is mainly wrappers to handle types more cleaner from caller, they are quick and simple to rewrite.

@am11
Copy link
Member Author

am11 commented Mar 25, 2022

Once the code gets removed I believe it will be hard for people not directly involved in the removal to start look for an old removed function that they don't know exists

Since this is version controlled, we can use git log -STHE_KEY to find the old code.

Discovering functions that exist is also hard. We deleted redundant utility functions, macros etc. in the past. Those existed because de-duplication aspect is often missed during the development and review.

I have found plenty of unused code in src/mono, that is unlinked, has no more than one occurrence in the entire repository (and those functions are without __attribute__((constructor)) etc.). Perhaps that code will never be used, perhaps someday some of it will; but keeping dead-code around for the reasons like "what if we need it tomorrow" in a busy repository is less than ideal. Code can be added when needed or a comment can explain the reason for its existence.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@jkotas
Copy link
Member

jkotas commented Mar 25, 2022

#66571

@jkotas jkotas merged commit 5978738 into dotnet:main Mar 25, 2022
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
* Delete unused native code in src/native

* Delete set_* functions from version types
@ghost ghost locked as resolved and limited conversation to collaborators Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants