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++ with class. 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 that struct members are public by default vs class 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.

In file included from ring.cpp:34:
./ring_impl.hpp:47:1: warning: class 'BFspan_impl' was previously declared as a
      struct; this is valid, but may result in linker errors under the Microsoft
      C++ ABI [-Wmismatched-tags]
class BFspan_impl;
^
./bifrost/ring.h:67:16: note: previous use is here
typedef struct BFspan_impl*        BFspan;
               ^

Mar 29 '22 20:03 league

Codecov Report

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.

Mar 29 '22 20:03 codecov-commenter

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?

Apr 15 '22 01:04 jaycedowell

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. :)

Apr 15 '22 02:04 league

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.

Apr 15 '22 02:04 jaycedowell

I'm going to try it...

Apr 15 '22 02:04 jaycedowell

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.

Apr 15 '22 02:04 league

I should probably verify that my fix doesn't just disable C++17 filesystem support universally.

Apr 15 '22 03:04 jaycedowell

Nope, the fix universally disables support. Back to the drawing board...

Apr 15 '22 13:04 jaycedowell

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:

disable the warning with a #pragma in the header file [I guess so]
use struct also in the cpp file, paying attention to explicitly mark all data with appropriate public/private modifiers [this is my solution]
wrap the class into a struct [blah, extra dots everywhere]
change the paradigm used to implement the C interface (using, for example, a dummy struct and reinterpret_cast) [blah]

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.

Apr 15 '22 14:04 league

Resolve class/struct distinction warning from clang

Codecov Report