xflouris / libpll

Phylogenetic Likelihood Library
GNU Affero General Public License v3.0
26 stars 6 forks source link

Anonymous union errors when including the new NNI/SPR rollback structures in C++ code #94

Closed cwhidden closed 8 years ago

cwhidden commented 8 years ago

We're starting to test the new tree rearrangements and came across the following errors when attempting to include the dev branch (f472b964735a402581417afd298e146f1989ff99) in c++ code:

src/pll.h:261:12: error: 'struct pll_utree_rb_s::::pll_utree_rb_spr_s' invalid; an anonymous union can only have non-static data members [-fpermissive] struct pll_utree_rb_spr_s ^ src/pll.h:272:12: error: 'struct pll_utree_rb_s::::pll_utree_rb_nni_s' invalid; an anonymous union can only have non-static data members [-fpermissive] struct pll_utree_rb_nni_s ^

Our fix was to remove the names from just the "struct NAME" portion:

typedef struct pll_utree_rb_s { int move_type; union { struct //pll_utree_rb_spr_s { pll_utree_t * p; pll_utree_t * r; pll_utree_t * rb; pll_utree_t * pnb; pll_utree_t * pnnb; double r_len; double pnb_len; double pnnb_len; } spr; struct //pll_utree_rb_nni_s { pll_utree_t * p; int nni_type; } nni; }; } pll_utree_rb_t;

I don't have any experience with the new anonymous union features in C/C++, but this code compiles fine in gcc and we successfully applied and rolled back an NNI operation. Is there a reason for the struct names? Do you know why this struct compiles in C but not in C++? I don't expect you to tailor libpll for C++, but if those names are unnecessary would you please consider removing them to maintain compatibility?

Please let me know if you require any more information.

xflouris commented 8 years ago

thanks for the feedback @cwhidden

I was aware that named types inside anonymous types is a C11 idiom, but I kept them in since they worked with C99 which is required for libpll. I'm now removing these forward declarations as you suggest, thanks!

The long answer to your question on why it compiles with a C compiler (gcc) and not with a C++ one (g++) in my opinion is the following:

First of all, it compiles with g++ as well, if you set the -fpermissive switch, however in general, it is not safe to use this switch. The issue here is that C and C++ are two different languages. One of the differences is that C++ automatically creates a type when you provide a prefix for a struct or union (e.g. struct X { ... };), while C will create a type only if you provide the typedef keyword. In this case, tthe problem with g++ is during the translation phase when it attempts to create a symbol table that includes (in our case) pll_utree_rb_spr_s and pll_utree_rb_nni_s but cannot match them since the union is unnamed. The solution to this problem (which would fix the compilation issue) would be to name the union in the following way:

union uu
{
  ...
} u;

This will fix the problem (if we also substitute rb-> with rb->u. in utree_moves.c) and the compiler will succeed in building a symbol table. In that case you would also be able (in C++ only) to declare in your programs variables of type pll_utree_rb_spr_s in the following way:

pll_utree_rb_s::uu::pll_utree_rb_spr_s x;

which at the moment you cannot because the union (uu in the named case) is un-named and breaks the symbol-table construction in C++.

Other than that libpll can be used in both C and C++ programs.

Feel free to close this issue.

cwhidden commented 8 years ago

Thanks @xflouris.