General cleanup in hyprland/workspaces

[zjeffer]

Did some cleanup in hyprland/workspaces:

• Renamed the CreateWindow class to WindowCreationPayload, to make it clearer it isn't a function but a class.
• Added const qualifiers and references to functions
• Added std::move in initializer lists to avoid making copies
• Put the WINDOW_CREATION_TIMEOUT variable in an anonymous namespace (it used to be statically defined inside the function it was used in, I don't understand why?)

On a related note, I also started a discussion here about adding a clang-tidy file to the repo: #2591. I'd love to hear your thoughts on this.

CC: @Syndelis @khaneliman @Anakael @MightyPlaza

[MightyPlaza]

hyprwm/Hyprland#3530

[zjeffer]

hyprwm/Hyprland#3530

@MightyPlaza Yep, that's the next step, for another PR :)
I first wanted to do some general cleaning up before implementing that.

[MightyPlaza]

just wanted to point it out since most code for persistent workspaces can be removed now

[Syndelis]

Put the WINDOW_CREATION_TIMEOUT variable in an anonymous namespace (it used to be statically defined inside the function it was used in, I don't understand why?)

It's just a personal preference, but if a "global" will only be used in one scope, I find it better to define it as a static const inside that scope. The reasoning is:

• It's not slower in any shape or form. In fact, it may as well compile down to the same thing: see it for yourself;
• It avoids needing to jump files in order to find the constant's value (it's right there in the same scope);
• Avoids raising the question "is this used somewhere else?", which directly leads you to global searching for its name. You can be a testimony for that: you literally took a look and realized it was only used inside of that function;
• Doesn't litter global namespace, which in turn avoids the need to define an anonymous namespace, reducing boilerplate code.

Given the above reasons, I'd say I don't understand why one would prefer a global variable over a scoped static const. Although I can't and wouldn't enforce you to keep it as it was, I do ask you to take my points into consideration and reconsider this change.

[zjeffer]

Here are the reasons I thought it was strange:

• We might use the constant in another function in the future, so my change takes that into account.
• If all constants used in a file are defined up top, it makes changing them easy without having to find the function they are used in. Say we have 5 constants, tuning these constants is really quick, without having to go to the functions they are used in.
• Where I work, we always use an anonymous namespace at the top to define constants, so it was a force of habit ;)

But your reasons are more logical in this case as it's only used in one place. I changed it back.

[zjeffer]

Just out of interest, could there be a (tiny) runtime performance difference when defining a static variable inside the function it is used in, if that function is called repeatedly? We're statically defining the WINDOW_CREATION_TIMEOUT variable in the update() function, which is called on every event. When running, does it check every iteration whether the static variable was defined yet? This obviously would make zero noticable difference in real-world usage, but I'm interested in how that works.

[Syndelis]

Just out of interest, could there be a (tiny) runtime performance difference when defining a static variable inside the function it is used in, if that function is called repeatedly? We're statically defining the WINDOW_CREATION_TIMEOUT variable in the update() function, which is called on every event. When running, does it check every iteration whether the static variable was defined yet? This obviously would make zero noticable difference in real-world usage, but I'm interested in how that works.

From the compiler explorer link, they should be compiled down to the same thing even with -O0. From the manual:

Most optimizations are completely disabled at -O0

#include <cstdio>

const int A = 10;

void with_global() {
  printf("A=%d\n", A);
}

void with_scoped() {
  static const int B = 20;
  printf("B=%d\n", B);
}

.LC0:
        .string "A=%d\n"
with_global():
        push    rbp
        mov     rbp, rsp
        mov     esi, 10
        mov     edi, OFFSET FLAT:.LC0
        mov     eax, 0
        call    printf
        nop
        pop     rbp
        ret
.LC1:
        .string "B=%d\n"
with_scoped():
        push    rbp
        mov     rbp, rsp
        mov     esi, 20
        mov     edi, OFFSET FLAT:.LC1
        mov     eax, 0
        call    printf
        nop
        pop     rbp
        ret

The only real difference is the scopes in which the compiler will allow you to use the variable

[Syndelis]

Great PR, and I look forward to the clang-tidy PR, too!

[zjeffer]

Just made some more changes, could you re-review? Thanks!

[zjeffer]

@Alexays ready to merge :)

[khaneliman]

Thank you for doing this... I didn't like how busy that method was.

[zjeffer]

This was also a hint by clang-tidy: it shows a warning if a function is too complex. Another reason to add it to the repo :)

[Syndelis]

Forgot to submit the review yesterday, my apologies!

[Alexays]

Thanks!