Open league opened 2 years ago
Merging #171 (3e3f580) into master (7790fdb) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #171 +/- ##
=======================================
Coverage 58.36% 58.36%
=======================================
Files 67 67
Lines 5753 5753
=======================================
Hits 3358 3358
Misses 2395 2395
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 7790fdb...3e3f580. Read the comment docs.
I'm torn about this. Addressing warnings is generally good but it feels kind of wrong to take all of the "class" out of the C++ code. Plus, I don't think we will be adding support for building on Windows. As an alternative idea, how would you feel about keeping things as class
and making use of #pragma clang diagnostic push
to disable these warnings?
I think we're not using clang anymore either, because of other problems that arose with c++17 and <filesystem>
! The github macos-latest picks up gcc, so I switched nix to doing that too. I think configure deduced that clang supports 17, but its linker didn't find filesystem definitions. That may ultimately be fixable – can still look into it – but sticking to gcc was the simpler solution.
Anyway, as long as clang is going unused this PR is kind of addressing "would-be" warnings. :)
Hey, on my laptop configure
detects and uses clang!
Seriously, though, I wonder if that original C++17/clang issue might have been related to the problem I ran into at https://github.com/ledatelescope/bifrost/pull/173/commits/d748a9a971cc9ea011e346e58ecaf0ad1c3d8705.
I'm going to try it...
Looks like you got it… nix is picking up clang again on mac and it went through. It was unclear to me, and a little surprising, when I discovered that github's macos-latest seemed to be preconfigured to use gcc... configure on my mac laptop was also picking up clang. So definitely for the best if we support both.
I should probably verify that my fix doesn't just disable C++17 filesystem support universally.
Nope, the fix universally disables support. Back to the drawing board...
Here's a small discussion I found that pinpoints the exact problem, with an array of solutions: https://stackoverflow.com/questions/41378623/c-interface-of-c-code-how-to-avoid-mismatched-tag-warning
and it looks like gcc may be adding a similar warning, though devs seem mixed on whether it means anything at all, or maybe both clang and gcc are jumping on an MS bandwagon that goes nowhere. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61339#c6
Anyway, the choices enumerated on SO are:
There's some support for choice 2 in the comments, and then an answer (with no votes) for choice 3.
IMO there's nothing wrong with using struct
this way in C++. Some people think of struct
as a C thing (plain old struct) and class
as having way more features, but in C++ they have the same features just different privacy defaults.
If you prefer to keep class
that's okay, but I might suggest just turn off the warning globally with CXXFLAGS containing -Wno-mismatched-tags
or a simple pragma in the relevant files (maybe don't bother with push/pop)...
71789714 seems to do the trick. GCC10 can handle these pragmas too, but the ifdefs hide them from gcc 7,8,9 which all seem to complain.
For these classes, we're declaring them a typedef with
struct NAME *
in a.h
file so that it can be used from C. But then the body is defined in C++ withclass
. The clang compiler on Mac complains (but says "I'm okay with this, but Microsoft might not be"). So it doesn't really matter, but I'd rather get rid of the warnings if it doesn't cost anything.It's okay to use
struct
in C++ with inheritance and all the other features of classes... the main difference is thatstruct
members are public by default vsclass
being private by default.So here we turn all of these that use the typedef struct pointer C into structs rather than classes, which silences the warnings.