unintialized error when building NeighborCommunicator.cpp

[Eric-Guiltinan]

Describe the bug
An uninitialized error when building GEOS. Perhaps when building NeighborCommunicator.cpp. See screenshot

To Reproduce
Build TPLs and then GEOS on latest ubuntu with gcc 11.4.0 and cmake 3.24.1

Screenshots

Platform (please complete the following information):

• Ubuntu 22.04.2 LTS
• Compiler: gcc 11.4.0
• GEOSX Version - latest version... cloned 8/3/23

Additional context
Built it in the same manner which was successful for us on 4/5/23

[joshua-white]

Hey @Eric-Guiltinan, the issue seems to be the FieldLocation enum is uninitialized here:
https://github.com/GEOS-DEV/GEOS/blob/514539bbd4a01f3f7c69ad684527aae6c86967dc/src/coreComponents/mesh/mpiCommunications/NeighborCommunicator.cpp#L507C1-L507C28

Could you try adding an additional Undefined element to the list here:
https://github.com/GEOS-DEV/GEOS/blob/514539bbd4a01f3f7c69ad684527aae6c86967dc/src/coreComponents/mesh/FieldIdentifiers.hpp#L31C1-L38C1

And then set FieldLocation location = FieldLocation::Undefined at line 507.

We should also maybe define default case behavior in the switch.

[Eric-Guiltinan]

That worked. We had to handle the undefined type in each of the switch statements as well. We just put them in as GEOS_ERROR ('Field Location is undefined') or something like that. Should we make a branch with the changes and submit a pull request?

[joshua-white]

Yes, please. It would be good to have some other folks look at this. I am a little surprised this happened, as I thought enums always had a default value (0), but at least we know the fix. Regardless, it is good for us to catch potentially undefined behavior.

[klevzoff]

Yeah all switches on enums must have a default case with a GEOS_ERROR() call in it. Even if we always initialize enums with a value (we should), the compiler cannot prove at the location of switch statement that a particular enum has one of the predefined values handled by the switch cases, so it issues a warning that we treat as an error (and it's a good thing). Adding a default case should be sufficient to silence it.

I'm just surprised we haven't caught this before. Our CI has gcc-11.2 which is a bit older than 11.4, so maybe the warning was added somewhere in between. IMO we should always have the newest available gcc and clang versions in our CI matrix so we can stay on top of new compiler diagnostics as they are added.

[cssherman]

@Eric-Guiltinan - Are you planning to submit a PR to address this issue, or should another dev do so?

[Eric-Guiltinan]

Sorry, yeah, we made the edits on another account and will submit a PR on Monday morning.

[Eric-Guiltinan]

I can't seem to push my branch to the repo. I think I don't have access. But I found the easiest fix for this was just to create a default case for the three switches in NearestNeighbor.cpp. I did not need to add the undefined case to FieldLocation as Josh suggested but maybe thats better.

[paveltomin]

@Eric-Guiltinan where is the PR?

[joshua-white]

@paveltomin, it seems easier for us to push a fix rather than @Eric-Guiltinan. this is on my todo list.

[paveltomin]

@joshua-white where is the PR?