chore: fix compiler warnings #618

mwerle · 2023-08-16T06:06:45Z

This PR fixes all warnings (mostly deprecations) in the codebase when building against QT5.15 using both clang and gcc.

It also ensures the codebase can still be compiled with versions of QT prior to 5.14 via a new compatibility header file qtsupport.h and, where that was not possible, via alternate code compilation paths via a #if..else..endif. The latter approach was required for the QSet initializer.

Out-of-scope:

deprecations/warnings in the dependencies

Changes:

add default to switch statements throwing an exception on unhandled case. This removes control reaches end of non-void function in many functions.
fix QT5.14 deprecations, which will also make it easier to move to QT6
- fix QSet initialization
- rename endl -> Qt::endl
- rename QString::SkipEmptyParts -> Qt::SkipEmptyParts
- and more...
Scintilla editor
- add missing override
- replace deprecated functions
Settings
- fix deprecated QDir assignment
- code-style: replace assignment with initializers (to match QDir in this file)

NOTE: did not fix the deprecated usage of QProcess, as that requires a bit more throught as to how we should handle the list of configured commands and their arguments.

Testing:

The unit tests pass.
anecdotal (ie, own usage) testing showed no issues.

Murmele · 2023-08-22T12:24:14Z

Thanks, I will have a look into it.

mwerle · 2023-08-22T23:47:47Z

G'day, apologies for the failed formatting; I didn't notice the script..

Is there a way I can run the CI tasks locally prior to sending a PR? I did also try to trigger them via GitHub after I created the PR but couldn't figure out how.

Murmele · 2023-08-23T09:28:14Z

G'day, apologies for the failed formatting; I didn't notice the script..

Is there a way I can run the CI tasks locally prior to sending a PR? I did also try to trigger them via GitHub after I created the PR but couldn't figure out how.

You can run the unittests by entering the build folder and execute ninja test or ctest

You can use the format script to format the code and then comitting it

mwerle · 2023-08-23T14:52:18Z

You can run the unittests by entering the build folder and execute ninja test or ctest

You can use the format script to format the code and then comitting it

Sure, but the CI does more such as performing builds for a variety of platforms and creating the Flatpak etc.

Hmm, formatting is still failing? But I ran clang-format locally and committed all the requested changes and re-running it does not show up any new deviations.

EDIT: What's more, it's complaining about test/Setting.cpp, which is a file I never modified to start off with and whose latest changes are all formatting related.

Murmele · 2023-08-23T20:20:02Z

This is because of the clang-format version which is 13, it makes problems with clang-format 14 ...

mwerle · 2023-08-24T00:06:21Z

This is because of the clang-format version which is 13, it makes problems with clang-format 14 ...

Right, well, your cl-format.sh always looked for the latest version. I've modified it to ensure it uses v13. Also fixed the cmake-format to use a python venv; IMHO poor practise to install stuff into a developers' machine globally.

Finally, added a configuration to run clang-format v13 as a pre-commit hook. "Works For Me" but might require additional testing on a variety of build environments. It's optional to use in either case.

.pre-commit-config.yaml

src/app/CustomTheme.cpp

Murmele · 2023-08-28T15:06:00Z

default.patch

mwerle · 2023-08-29T05:23:12Z

So after looking into it a it more, I must say I'm not keen on the -Werror=switch-enum switch. It forces enumerating all items in a switch statement even if there is a default. IMHO this just adds noise to the code and makes it less readable.

As far as I see it, there are basically 2 situations:

developer wants to handle every case explicitly, meaning there should not be a default. This allows the compiler to warn if a case was forgotten, or during later development when a new case is added to ensure it is handled in all affected switch statements. This is primarily when the developer controls both the enum as well as the logic.
developer only wants to handle a subset of cases. In this case the developer should not have to list all the cases which are of no interest but instead use default. This is often the case when the enum comes from a third-party source such as an external library.

So I've changed the switch to -Werror=switch, and I've also moved all my throw statements outside the switch statements; in retrospect having them in the default clause was a poor choice by myself initially. This also means that I did not apply your patch and, in fact, deleted several items from some switch statements and replaced them with a default entry instead.

Please let me know if some/all of your patch should be applied after all and I'll go through it again.

I've also briefly enabled -Wall and fixed a few more warnings. -Wextra was just too much for now. Ultimately I would encourage the project to enable all warnings and aim towards a zero-warning compilation policy. While many warnings are advisory (and sometimes a right-royal pain to remove for questionable benefit), a few are actually indicative of real code problems. It's difficult to find a new problem if it is swamped by dozens/hundreds of innocuous warnings.

Finally, I rebased back on master - please let me know if you prefer if I should merge instead of rebasing.

Murmele · 2023-08-29T06:51:31Z

Hi @mwerle sorry that I did not apply my patch my self, but I was not able to do from the codespace for some reason.
Yes I agree with you, sometimes Werror=switch-enum is to strict, but for me the default is like a joker and if you add a new enum you might forgett to implement somewhere the case. But neverthless right now we can let it as it is, thank you for your great work!

mwerle · 2023-08-29T07:52:47Z

No worries re the patch. I'm sorry, I didn't go through it in detail, but mostly it seemed to be about adding a default case to many of the switch statements. As I argued, adding a default actually prevents the compiler from warning about missing options (unless you use the -Wswitch-enum flag).

If you'd still like me to apply it, I'll of course do so. And apologies for taking up so much of your time on a "useless" refactor.. hoping to contribute some more useful changes in the future :)

* add default to switch statements throwing an exception on unhandled case. This removes `control reaches end of non-void function` in many functions. * fix QT5.14 deprecations, which will also make it easier to move to QT6 - fix QSet initialization - rename `endl` -> `Qt::endl` - rename `QString::SkipEmptyParts` -> `Qt::SkipEmptyParts` - and more... * Scintilla editor - add missing `override` - replace deprecated functions * Settings - fix deprecated QDir assignment - code-style: replace assignment with initializers (to match QDir in this file)

The refactor to remove deprecated symbols also broke compatibility with versions of QT prior to QT5.14. This commit adds back support for earlier versions of QT. Mostly this is via the new "qtsupport.h" header file, but for supporting the changed `QSet` constructor we had to put the changes into each source file. This is, of course, not optimal.

We used v14 of clang-format in commit 2dfb5a4 which introduced unwanted changes. Revert to the formatting enforced by v13 of clang-format.

Adding a configuration to invoke clang-format v13 on all modified files as part of the commit process, and a new script file `setup-env.sh` to manage installing the pre-commit hook. The hook is installed using a local python venv to ensure the correct version of clang-format is used. This approach also enables the hook to be installed on any platform which supports python although the setup script is limited to platforms which support bash.

Added a note to the `Contributing` section to invoke `cl-format.sh` and to run the tests prior to creating any PR.

… fix remaining issues. Moved the `throw` statements introduced in an earlier commit to after the `switch` statement and removed the `default` which could mask an unhandled case. Changed some situations where every enum item was listed, but fell through to a default action by replacing the explicitly enumerated items with a `default`. This was primarily where the enum was defined in an external library.

Temporarily enabled `-Wall` and fixed more warnings, but there are several uninitialized variables usages which may require review by the code author in order to ensure the refactor is correct so we leave these additional warnings disabled for now. Fixed: * removed unused lambda captures * removed unused variables * fixed sign comparisons between variables

The lambda captures `model`, but in the code, `mModel` is used. Either the capture is superfluous, or the usage is incorrect.

src/editor/TextEditor.cpp

This variable is actually required for Flatpak builds, so revert removing it. But to avoid compiler warnings, guard it with a precompiler check.

mwerle · 2023-08-31T11:35:06Z

Hmm.... for some reason my git pre-commit hook isn't being invoked; at least, not when using gittyup. I did notice it being invoked when using the command-line a few days ago. Something to investigate further.

Murmele · 2023-08-31T11:53:11Z

No problem, you can fix it in another MR. I will merge it as soon as the CI is finished. Thank you!

Murmele self-requested a review August 22, 2023 12:23

mwerle mentioned this pull request Aug 25, 2023

Feature/rename branch menu #621

Merged

Murmele reviewed Aug 27, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

src/app/CustomTheme.cpp Outdated Show resolved Hide resolved

mwerle force-pushed the feature/clean_build branch from 989ef27 to 1d73792 Compare August 29, 2023 05:18

mwerle force-pushed the feature/clean_build branch from fac4e01 to e11976a Compare August 31, 2023 00:42

mwerle and others added 11 commits August 31, 2023 09:43

chore: clang-format recently modified files.

3ffdbac

chore: revert formatting changes of Setting.cpp

d729e63

We used v14 of clang-format in commit 2dfb5a4 which introduced unwanted changes. Revert to the formatting enforced by v13 of clang-format.

doc: update README - contributing

ea56be2

Added a note to the `Contributing` section to invoke `cl-format.sh` and to run the tests prior to creating any PR.

enable -Werror=switch-enum

98eb73b

question: CommitList - correct variable to use?

6ceaa40

The lambda captures `model`, but in the code, `mModel` is used. Either the capture is superfluous, or the usage is incorrect.

chore: forgot clang-format on recent changes

fb9a799

mwerle force-pushed the feature/clean_build branch from e11976a to fb9a799 Compare August 31, 2023 00:43

Murmele requested changes Aug 31, 2023

View reviewed changes

src/editor/TextEditor.cpp Show resolved Hide resolved

fix: revert removing global textHeightFactorCheckBoxSize

6cf922f

This variable is actually required for Flatpak builds, so revert removing it. But to avoid compiler warnings, guard it with a precompiler check.

mwerle force-pushed the feature/clean_build branch from ab2192d to 6cf922f Compare August 31, 2023 11:34

Murmele self-requested a review August 31, 2023 12:19

Murmele approved these changes Aug 31, 2023

View reviewed changes

Murmele merged commit 820c0fd into Murmele:master Aug 31, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fix compiler warnings #618

chore: fix compiler warnings #618

mwerle commented Aug 16, 2023 •

edited

Loading

Murmele commented Aug 22, 2023

mwerle commented Aug 22, 2023

Murmele commented Aug 23, 2023

mwerle commented Aug 23, 2023 •

edited

Loading

Murmele commented Aug 23, 2023

mwerle commented Aug 24, 2023

Murmele commented Aug 28, 2023

mwerle commented Aug 29, 2023 •

edited

Loading

Murmele commented Aug 29, 2023

mwerle commented Aug 29, 2023

mwerle commented Aug 31, 2023

Murmele commented Aug 31, 2023

chore: fix compiler warnings #618

chore: fix compiler warnings #618

Conversation

mwerle commented Aug 16, 2023 • edited Loading

Murmele commented Aug 22, 2023

mwerle commented Aug 22, 2023

Murmele commented Aug 23, 2023

mwerle commented Aug 23, 2023 • edited Loading

Murmele commented Aug 23, 2023

mwerle commented Aug 24, 2023

Murmele commented Aug 28, 2023

mwerle commented Aug 29, 2023 • edited Loading

Murmele commented Aug 29, 2023

mwerle commented Aug 29, 2023

mwerle commented Aug 31, 2023

Murmele commented Aug 31, 2023

mwerle commented Aug 16, 2023 •

edited

Loading

mwerle commented Aug 23, 2023 •

edited

Loading

mwerle commented Aug 29, 2023 •

edited

Loading