[Code Cleanup] Cleaned up some header files and move a bit of debugging logic to cmake#7677
[Code Cleanup] Cleaned up some header files and move a bit of debugging logic to cmake#7677Rossmaxx merged 49 commits intoLMMS:masterfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Rossmaxx
changed the title
lmms_basics.h and simplify the LMMS_STRINGIFY macro to be used at call sites.lmms_basics.h and scope it down to type declaration
|
Do note that the diff itself is small. Most of the thing is due to the file rename in the #include statement |
|
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. |
|
I'll fix the declaration later. |
|
Seems like I got myself into a rabbit hole with this refactoring |
Rossmaxx
changed the title
lmms_basics.h and scope it down to type declaration
|
I'm done now. |
Rossmaxx
force-pushed
the
clean-lmms-basics
branch
from
dff51ab to
9e25b29
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Rossmaxx
force-pushed
the
clean-lmms-basics
branch
from
a339a32 to
3fb24f2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Rossmaxx
force-pushed
the
clean-lmms-basics
branch
from
72fd59d to
8cf8b36
Compare
|
I'll undraft the PR once I manage to fix the history. The work is done for now. |
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. |
|
I found a way to fix it. I'll reorder commits using an interactive rebase. But it is a bit effort consuming tho. |
Rossmaxx
force-pushed
the
clean-lmms-basics
branch
2 times, most recently
from
756c90e to
3d42227
Compare
szeli1
left a comment
szeli1
left a comment
There was a problem hiding this comment.
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
|
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 |
|
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. |
|
@rubiefawn can i get a green tick atleast now? |
@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. |
Well, you got a point too. I will wait for a 2nd opinion before reverting. |
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. |
You mean a 3rd? |
|
@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. |
…ng logic to cmake (LMMS#7677) * 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
7 participants
lmms_basics.htoLmmsTypes.hand scoped it down for that purpose.LMMS_STRINGIFYmacro to it's own header.debug.hheader file and use cmake to handle the macro logic.