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

Split PropertyInfo and MethodInfo off from Object into their own separate files. #88831

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nlupugla
Copy link
Contributor

@nlupugla nlupugla commented Feb 25, 2024

To avoid cyclic dependencies in a different PR I am working on (#82198), I need to be able to include PropertyInfo and MethodInfo without also including Object. Additionally, separating out these structs helps keep Object.h/cpp focused on just Object stuff.

@nlupugla nlupugla requested review from a team as code owners February 25, 2024 19:32
@KoBeWi KoBeWi added this to the 4.x milestone Feb 25, 2024
@nlupugla nlupugla force-pushed the spin-off-object-helper-structs-into-seperate-files branch from 527fa29 to 4c78b0a Compare February 25, 2024 19:45
@nlupugla
Copy link
Contributor Author

Hmm, I'm noticing that the Linux build is failing because Connection already seems to be defined in some third-party file. Should I put connection.h/cpp in maybe namespace Signal to avoid the conflict? Alternatively, I could move Connection to be inside Callable. The only other option I can think of really is to rename Connection to something like SignalConnection.

@nlupugla
Copy link
Contributor Author

I actually managed to find a different solution so that structs can work without relying on splitting PropertyInfo, MethodInfo, and Connection into separate files. I think splitting off PropertyInfo and MethodInfo into their own files is still good for code hygiene, but I'll leave Connection attached to Object to avoid the namespace conflicts over Connection.

If others don't like the change of splitting of PropertyInfo and MethodInfo, I'm fine with closing this PR.

@nlupugla nlupugla force-pushed the spin-off-object-helper-structs-into-seperate-files branch from 4c78b0a to 244a8ef Compare April 9, 2024 19:37
@nlupugla nlupugla changed the title Split PropertyInfo, MethodInfo, and Connection off from Object into their own separate files. Split PropertyInfo and MethodInfo off from Object into their own separate files. Apr 9, 2024
@nlupugla
Copy link
Contributor Author

nlupugla commented Apr 9, 2024

I've resolved the conflicts on this branch and left the Connection struct as part of Object.

@nlupugla nlupugla force-pushed the spin-off-object-helper-structs-into-seperate-files branch from 244a8ef to 0f35529 Compare April 12, 2024 03:06
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait. For me this is sensible, but someone from core should take a look at this as well.
Should be safe to merge, as I just checked that there were no changes of MethodInfo/PropertyInfo within the last few months.

@akien-mga
Copy link
Member

Makes sense to me conceptually. Could use a rebase to make sure it still works.

core/object/object.cpp Outdated Show resolved Hide resolved
@nlupugla nlupugla force-pushed the spin-off-object-helper-structs-into-seperate-files branch from 0f35529 to da81925 Compare September 8, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants