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

[Code Cleanup] Cleaned up some header files and move a bit of debugging logic to cmake #7677

Merged
merged 49 commits into from
Mar 24, 2025

Conversation

Rossmaxx
Copy link
Contributor

@Rossmaxx Rossmaxx commented Feb 2, 2025

  • Renamed lmms_basics.h to LmmsTypes.h and scoped it down for that purpose.
  • Shifted the LMMS_STRINGIFY macro to it's own header.
  • Removed the debug.h header file and use cmake to handle the macro logic.
  • Remove some unused headers and include directives

@Rossmaxx

This comment was marked as outdated.

@Rossmaxx Rossmaxx changed the title Cleanup lmms_basics.h and simplify the LMMS_STRINGIFY macro to be used at call sites. Rename lmms_basics.h and scope it down to type declaration Feb 14, 2025
@Rossmaxx
Copy link
Contributor Author

Do note that the diff itself is small. Most of the thing is due to the file rename in the #include statement

@Rossmaxx Rossmaxx marked this pull request as ready for review February 14, 2025 15:32
@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Feb 14, 2025

Now i should hunt down any unused includes over here, but I don't really have the insanity to do that manually.

Edit: using iwyu, I could somewhat automate this process.

@Rossmaxx
Copy link
Contributor Author

I'll fix the declaration later.

@Rossmaxx
Copy link
Contributor Author

Seems like I got myself into a rabbit hole with this refactoring

@Rossmaxx Rossmaxx changed the title Rename lmms_basics.h and scope it down to type declaration [Code Cleanup] Cleaned up some header files and move a bit of debugging logic to cmake Feb 22, 2025
@Rossmaxx
Copy link
Contributor Author

I'm done now.

@Rossmaxx

This comment was marked as outdated.

@Rossmaxx Rossmaxx marked this pull request as draft March 1, 2025 07:10
@Rossmaxx Rossmaxx force-pushed the clean-lmms-basics branch from a339a32 to 3fb24f2 Compare March 3, 2025 14:49
@Rossmaxx

This comment was marked as outdated.

@Rossmaxx Rossmaxx force-pushed the clean-lmms-basics branch from 72fd59d to 8cf8b36 Compare March 3, 2025 15:23
@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Mar 3, 2025

I'll undraft the PR once I manage to fix the history. The work is done for now.

@tresf
Copy link
Member

tresf commented Mar 3, 2025

@tresf any idea on fixing this?

When I get into a pickle like this, I take the lazy route and PR against a test branch that's even with master, squash everything down, force that commit back. For this reason, I'm probably not a good person to ask. Last time my efforts were scrutinized #7726. Perhaps someone with more experience in this area can help.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Mar 3, 2025

I found a way to fix it. I'll reorder commits using an interactive rebase. But it is a bit effort consuming tho.

@Rossmaxx Rossmaxx force-pushed the clean-lmms-basics branch 2 times, most recently from 756c90e to 3d42227 Compare March 3, 2025 16:40
Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

Since you renamed lmms_basic.h to LmmsTypes.h, why not do something similar for lmms_constants.h and lmms_math.h?

@Rossmaxx
Copy link
Contributor Author

Since you renamed lmms_basic.h to LmmsTypes.h, why not do something similar for lmms_constants.h and lmms_math.h?

I could, but mind if I defer, the PR is getting a bit too large already. I'll do the other changes now.

Copy link
Contributor

@szeli1 szeli1 left a comment

Choose a reason for hiding this comment

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

I tested this PR and it doesn't seem like it causes any issues or bugs.

I couldn't find any problems with the code.

I think a future improvement could be if the Spectrum analyzer FFT code was moved to a common lmms class, since it is common between the eq and SpectrumAnalyzer

@Rossmaxx
Copy link
Contributor Author

Thanks for the heads up @szeli1. I could do with another review from someone with write access, and I am safe to hit the button. @sakertooth mind following up? Need your opinion on deferring the other renames too

@sakertooth
Copy link
Contributor

Should we change the name of the changed files from snake case to camel case as well?

@Rossmaxx
Copy link
Contributor Author

Should we change the name of the changed files from snake case to camel case as well?

No need. That's a bit too much.

@Rossmaxx
Copy link
Contributor Author

@rubiefawn can i get a green tick atleast now?

@tresf
Copy link
Member

tresf commented Mar 17, 2025

I tested this PR and it doesn't seem like it causes any issues or bugs.

I couldn't find any problems with the code.

I think a future improvement could be if the Spectrum analyzer FFT code was moved to a common lmms class, since it is common between the eq and SpectrumAnalyzer

@Rossmaxx Why was this recommendation ignored? I agree with @szeli1, just because a value is only used in one place doesn't mean it shouldn't be in a shared header. This moving things like audible frequencies into plugin-specific headers does not make sense.

@Rossmaxx
Copy link
Contributor Author

This moving things like audible frequencies into plugin-specific headers does not make sense.

Well, you got a point too. I will wait for a 2nd opinion before reverting.

@Rossmaxx
Copy link
Contributor Author

why was this recommendation ignored?

As to ignoring that comment, I don't really understand that code as of now, and though i could dabble a bit I don't want to create any regressions.

@tresf
Copy link
Member

tresf commented Mar 17, 2025

This moving things like audible frequencies into plugin-specific headers does not make sense.

Well, you got a point too. I will wait for a 2nd opinion before reverting.

You mean a 3rd?

@Rossmaxx
Copy link
Contributor Author

@tresf I have reverted the spectrum analyser stuff, it's bloating the diff for no real benefit. Also I'm ignoring the fftw comment for now. The PR is ready for merge.

@Rossmaxx Rossmaxx merged commit 7367750 into LMMS:master Mar 24, 2025
10 checks passed
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.

7 participants