Bug Fix: Fix QRegularExpression-related compile error

[hoffie]

Short description of changes

b31e650 updated regexps to QRegularExpression for Qt6 compatibility, but did not include QRegularExpression.h.

While this has apparently not been a problem in most environments, it breaks the build when using CONFIG+=headless (CONFIG+="headless nosound" is unaffected).

This PR adds the includes.

Context: Fixes an issue?

Fixes #2272

Does this change need documentation? What needs to be documented and how?

No

Status of this Pull Request

Ready

What is missing until this pull request can be merged?

Reviews.

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[hoffie]

@corrados Can you please try the code from this PR in your Raspbian environment? Thanks :)

[hoffie]

I've checked for other places which use QRegularExpression and have added the include there as well.

[softins]

It's interesting, because when doing the original change to QRegularExpression, I actually did some examination of the Qt headers, and concluded that the explicit #include <QRegularExpression> wasn't needed, as nested includes already pulled in the required definitions. This was then borne out by: 1. my successful compilation on Ubuntu, CentOS and Pi (both previous and current OS versions), and 2. compilation by the CI succeeding too, on all platforms.

So I'm not yet convinced that this PR is necessary, and would like to reproduce and understand Volker's compilation error before deciding whether to approve it for inclusion. My suspicion is that Volker's issue has a different cause.

[hoffie]

I've looked some further into this. I'm pretty convinced that QRegularExpression is an implicit include, which we should avoid, I think? The docs mentions the header explicitly, so I think we should include it.

g++ is able to explain where it comes from, i.e. why it works most of the time despite the missing include:

$ make util.o CXXFLAGS="-H" |& grep qregularexpression -B6
.. /usr/include/qt/QtWidgets/QComboBox
... /usr/include/qt/QtWidgets/qcombobox.h
.... /usr/include/qt/QtWidgets/qabstractitemdelegate.h
..... /usr/include/qt/QtWidgets/qstyleoption.h
...... /usr/include/qt/QtWidgets/qabstractspinbox.h
....... /usr/include/qt/QtGui/qvalidator.h
........ /usr/include/qt/QtCore/qregularexpression.h

[... there is another path via QtConcurrent which incldues all of QtCore, but this shouldn't matter here]

As expected, the problem is reproducible with CONFIG+=headless. It seems like we are not doing that on PRs?
On PRs, we build with CONFIG+=nosound headless. Adding nosound makes the problem go away, probably because it is somehow triggered from client-code. However, this configuration is not listed as unsupported and Raspberries seem to actively use/encourage that.

[softins]

I have just tried a clone of current master on a fresh Raspberry Pi system (which was actually unexpectedly Debian 10, not 11, because I hadn't updated the imager on my PC). When doing cd distributions; ./raspijamulus.sh, I got the same error, so it has been there all along, and is not new in Raspbian 11.

So it looks like specifying headless but omitting nosound leaves out some include that was implicitly pulling in qregularexpression.h. I'll pull your PR on the Pi and try again to see if the explicit include fixes it.

[softins]

OK, this change does fix the build on my Pi with CONFIG+=headless, so happy to approve.

[pljones]

Do we need all three or just the util.h one?

[hoffie]

Do we need all three or just the util.h one?

For fixing the compile error with CONFIG+=headless, src/util.h is sufficient

Do you suggest we should drop the other changes and if so, why? I've got very little C++ experience, however, when I looked around I stumbled over best-practice guides such as the C++ style guide by Google which suggests not to rely on transitive inclusions.

[softins]

Do we need all three or just the util.h one?

I think all three. It appears that best practice, for the way Jamulus code is structured, would be that for any foo.cpp that uses QRegularExpression, there should be a #include <QRegularExpression> in foo.h.

[pljones]

If the regexp usage is in chatdlg.cpp and connectdlg.cpp, then yes, their .h files should include QRegularExpression.

Is the regexp usage there and, if so, should it be there? If we're okay with that, then I'm okay with the change. (Just the PR doesn't have much context supplied.)

[hoffie]

If the regexp usage is in chatdlg.cpp and connectdlg.cpp, then yes, their .h files should include QRegularExpression.

Yes, it is:

$ grep QRegularExpression src/* -ri
src/chatdlg.cpp:    if ( !strChatText.contains ( QRegularExpression ( "href\\s*=|src\\s*=" ) ) )
src/chatdlg.cpp:        strChatText.replace ( QRegularExpression ( "(https?://\\S+(?<!" PUNCT_NOEND_URL ")(?<!\\?" PUNCT_NOEND_URL "))" ),
src/connectdlg.cpp:                QRegularExpressionMatchIterator reMatchIt = QRegularExpression ( "[A-Z][^A-Z]*" ).globalMatch ( strCountryToString );
src/util.cpp:    QRegularExpression rx1 ( "^\\[([^]]*)\\](?::(\\d+))?$" ); // [addr4or6] or [addr4or6]:port
src/util.cpp:    QRegularExpression rx2 ( "^([^:]*)(?::(\\d+))?$" );       // addr4 or addr4:port or host or host:port
src/util.cpp:    QRegularExpressionMatch rx1match = rx1.match ( strAddress );
src/util.cpp:    QRegularExpressionMatch rx2match = rx2.match ( strAddress );

Is the regexp usage there and, if so, should it be there? If we're okay with that, then I'm okay with the change.

Well, the functions are there, are being used and I don't see an obvious alternative, so yes, they should be there.

[hoffie]

I've updated the commit text to reflect the final analysis (CONFIG+=headless is the problem, not Raspberry per-se).
As this PR fixes a bug which is in no released version, I have not added a new Changelog entry, but have rather extended the existing one for #2124).