Refactor/gh13222 util deprecate

[Swiftb0y]

as requested in #13222

One primary issue is that marking entire classes [[deprecated]] will result in build failures due to -Wdeprecated -Werror. So marking them deprecated formally would require their removal in the same step. That's why I only made them deprecated in the form of a comment.

[Swiftb0y]

needs some more testing first

[fwcd]

One primary issue is that marking entire classes [[deprecated]] will result in build failures due to -Wdeprecated -Werror

Can we use -Wno-error=deprecated-declarations etc.? (IMO It would still be useful to have at least a warning)

[Swiftb0y]

probably yeah. I'll look into it.

[daschuer]

We may use codespell or similar with the list of deprecated functions and classes to create a warning during pre-commit. Will this work?

A warning for deprected function that we want to keep in the code for now will introduce a lot of noise. Contributors will start to look into it on one hand and we may get blind for other concerning warnings.

[Swiftb0y]

I don't think a purely lexical solution will work. I prefer @fwcd's suggestion. I'll see how many warnings it creates and evaluate from there how easy it will be to remove them.

[fwcd]

A warning for deprected function that we want to keep in the code for now will introduce a lot of noise. Contributors will start to look into it on one hand and we may get blind for other concerning warnings.

If the function is deprecated and is no longer to be used, having contributors look into it is IMO a good thing, given that those call sites should be refactored anyway. This is the purpose of these warnings, to draw attention to code snippets that should be updated. If we really need to keep it around, we may look into suppressing it on a case-by-case basis.

[Swiftb0y]

apart from the formal deprecation warnings, this is ready for review.

[daschuer]

If the function is deprecated and is no longer to be used, having contributors look into it is IMO a good thing

I strongly disagree. The contributor should always start over with a clean build without any warning. This is important to keep the barrier for new contributors low. A new contributor cannot understand what it means, unless we define a receipt for a fix.

We may rather work on this as the core team in a branch with warnings enabled. I also consider it not as an urgent task. Because the old code is known good and tested. Every change involves a degree of a regression risk. I don't like to review PRs with new code and deprecation warning fixes.

[fwcd]

The contributor should always start over with a clean build without any warning

Yeah, that's fair and it makes sense to me to reduce the number of warnings as much as possible, hence my idea that we could suppress warnings as needed (though the downside is that C++ doesn't seem to have a standard mechanism for this and adding different compiler-specific pragmas might add even more noise)

Do we have an idea of how many such warnings we would get? If the number is manageable, maybe we could fix the issues on main where we have enough time to test them?

[acolombier]

Thanks for looking into this. Having warning, which shows as annotation on PR or in the pre-commit would be very valuable I think

[Swiftb0y]

src/widget/paintable.cpp requires also #include

what header would that be? don't see any class from <memory> being used in Paintable (on 2.4)

[Swiftb0y]

Do we have an idea of how many such warnings we would get?

mixxx::Duration has 414 results in 123 files, and ReadableSlice/WriteableSlice ~59 results in 19 files.

I don't like to review PRs with new code and deprecation warning fixes.

I disagree. I think applying the "Boy Scout Rule" (Robert C. Martin) is an important tactic in combating code rot. Outlawing it would result in PRs that is either just a feature (that is explicitly not improving code quality, especially because we sometimes even enforce using outdated practices "for consistency") or PRs that are just refactorings and cleanup (for which ppl don't have incentive to review because it doesn't result in a new exciting feature). Do you see the dilemma here?

[Swiftb0y]

If you don't like the warnings, we can either keep it deprecated "informally" as I have done now, or I can issue another pull request that removes the deprecated classes entirely. The latter approach resulting in a PR that is likely to get neglected because of the reasons I mentioned in the previous comments.

[daschuer]

src/widget/paintable.cpp requires also #include

what header would that be? don't see any class from being used in Paintable (on 2.4)

https://github.com/mixxxdj/mixxx/blob/da261b6f006141fbca8b10490e73ad2211703d20/src/widget/paintable.cpp#L58

[daschuer]

The whole question boils down to:

• Do we want to remove the deprecated functions
• Who should do this.

If yes and one has interest to do it, than go ahead. If no, than don't impose the work to others by a warning.

In the later case we don't have yet a convenient way to enable the warning only for new code. We may enable the warning and disable it for all these 123 files.

But I think the current solution in this PR is good enough for now.

[Swiftb0y]

Do we want to remove the deprecated functions

IMO yes.

Who should do this.

I would start an attempt. I may not be able to succeed entirely, but I'm fairly confident a large number of occurrences could be eliminated at which point disabling warnings for select files would become feasible.

But I think the current solution in this PR is good enough for now.

Thank you. I agree for now. At least with the informal deprecation, people have the chance to know that a types use is discouraged, even if they aren't scolded for it by the compiler.

https://github.com/mixxxdj/mixxx/pull/13223#issuecomment-2105717856

Whoops thank you. fixed.

[Swiftb0y]

friendly ping

[daschuer]

I have not managed to test the ReferenceHolder changes. The 2.5-beta PRs have a higher prio for me. If you wish a quick merge, you may split that change out to another PR.

[Swiftb0y]

done #13240