Resolved compilation errors/warnings when building on Ubuntu 22.04.2 with clang

[iratahack]

A number of compilation errors and warnings addressed to allow building on Ubuntu 22.04.2 LTS (64-bit).

Environment

• clang (14.0.0-1ubuntu1.1) 14.0.0
• clang++ (14.0.0-1ubuntu1.1) 14.0.0
• cmake version 3.22.1

The original build log before modifications is attached for reference.

build.log

[pjft]

Thanks for sending this over.

We have a hard time reviewing this and accepting it as it stands, for a few reasons:

• It's not clear what warnings were you getting before, and what change fixes what;
• What compilation errors were you experiencing;

Finally, there are some indenting, logic and spacing changes that don't seem related to compilation errors and warnings, I imagine. Would love to get those separately explaining what they address, if possible.

Thanks, and happy Friday.

[iratahack]

@pjft , thanks for the feedback.

Let me clean this up and add more color around each change so you have some context for the changes.

IH.

[pjft]

Thank you!

[pjft]

Thank you for the detailed turnaround here. We will review this and get back to you, as there's a lot of ground to cover. :)

The good news is that indeed some of these were also needed for clang and @cmitu had also gone through similar changes for that purpose, on MacOS so it's great that you're looking into this for Ubuntu as well.

Thanks, and have a great weekend!

[tomaz82]

If the changes to StringUtil were to be vertically aligned and add 1 extra space before first = to match the |= that comes after then I'm all for merging this, the rest looks good.

[iratahack]

@tomaz82 , I think I got what you are saying. Please take a quick look. Thanks. IH.

[tomaz82]

@tomaz82 , I think I got what you are saying. Please take a quick look. Thanks. IH.

Yeah all looks good to me now, thanks!

[pjft]

Thanks both.

@iratahack could you squash all the commits and I'll merge them afterwards?

Thank you!

[iratahack]

@pjft , Done. Thanks! IH.

[cmitu]

@iratahack there are 2 additions which are producing some unnecessary logging, resulting from filling the default switch branch. I think these should be no-ops and not output anything.

1. https://github.com/RetroPie/EmulationStation/pull/820/files#diff-20dd5bee78ff69dd674ee9e1f690c6d1cbbed4faf238fe280aa8d809fd15a2d1R741 is issued when processing a custom collection, (Unknown type: 0 ), but this is not an error/warning since it just means that the file is not part of a pre-defined collection.
2. https://github.com/RetroPie/EmulationStation/pull/820/files#diff-72ecf0e45a89c7a1328dae3f44aeb6c44d67678ea4827e54326e901cd0ee0ea1R89 is issued when doing a normal exit from ES, which is also misleading - rather than Unknown, the case here is that a reboot/shutdown type is not set (i.e. user hasn't chosen either to reboot nor to shutdown the system, but rather just exited the program). I think this shouldn't be logged at all.

Similarly, the logging message from https://github.com/RetroPie/EmulationStation/pull/820/files#diff-b2fb93b6495366ee4f10f266fdaa37f22a0d8e181d492291bc525fbe61c31300R175 could be expanded to Unknown Filter type to make it more explicit.

Thank you for the additions, clang should be able to compile ES with them.

[iratahack]

@cmitu, appreciate the feedback! I have pushed the changes for the default cases as you suggested. IH.

[cmitu]

@iratahack the changes are fine, thanks for taking them into consideration.

There's just one thing - related to the libvlc version bump. Can you modify findVLC.cmake (here) and bump the version to 3.0.0 ?

[iratahack]

@cmitu , what about this line?

https://github.com/RetroPie/EmulationStation/blob/f4c3815b44d14593c8708c116d1678850afd04b0/CMake/Packages/FindVLC.cmake#L26

I pushed an update for it. IH.

[cmitu]

@cmitu , what about this line?

You're right, both need to be changed. Looks fine now, thanks for the changes.

@pjft I think it's ok to merge this now.

[iratahack]

Do you want me to squash the 2 commits?

[pjft]

Yes please, if possible. I'll try once again to compile on the pi here. @cmitu do you confirm it compiles in the current RetroPie version? Thank you both!

[cmitu]

I'll try once again to compile on the pi here.

It should fail during the configuration phase, due to the new libvlc >= 3.0.0 check (assuming you're using your old system, based on jessie).

@cmitu do you confirm it compiles in the current RetroPie version? Thank you both!

Yes, compiles on RaspiOS buster/bullseye and Ubuntu 18.04.6 LTS (which is in theory the earliest Ubuntu variant we support in RetroPie).

[pjft]

I confirm it failed. I'm on stretch.

I then updated VLC and it now compiles. The install script depends on libvlc-dev, so I assume it'll update automatically to the latest version when we run it, and I assume that the current images already come with 3.0 or above.

Thanks all!

[pjft]

Thank you!

[pjft]

@iratahack I just merged this on master, which was kind of where I thought it had been pushed to. Can you check if it also compiles without warnings, as there were some updates compared to this branch? Thanks.

[iratahack]

@pjft , I'm seeing a few override warnings on master. I can create a pull request if you like.

In file included from /home/devel/git/EmulationStation/es-app/src/views/gamelist/GridGameListView.cpp:1:
/home/devel/git/EmulationStation/es-app/src/views/gamelist/GridGameListView.h:24:15: warning: 'setViewportTop' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void setViewportTop(int index) { ; }
                     ^
/home/devel/git/EmulationStation/es-app/src/views/gamelist/ISimpleGameListView.h:28:15: note: overridden virtual function is here
        virtual void setViewportTop(int index) override = 0;
                     ^
In file included from /home/devel/git/EmulationStation/es-app/src/views/gamelist/GridGameListView.cpp:1:
/home/devel/git/EmulationStation/es-app/src/views/gamelist/GridGameListView.h:25:14: warning: 'getViewportTop' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual int getViewportTop() { return -1; }
                    ^
/home/devel/git/EmulationStation/es-app/src/views/gamelist/ISimpleGameListView.h:27:14: note: overridden virtual function is here
        virtual int getViewportTop() override = 0;
                    ^

IH.

[pjft]

Just these? I tried to fix some of the new ones but clearly this passed by me. Sure enough, if you want to send one to master it'd be great. Thank you!