Skip to content

Commit

Permalink
Don't predeclare a struct as a class (#114)
Browse files Browse the repository at this point in the history
Holy moly this took a long time to debug.

MSVC gives absolutely zero warnings about if a struct/class is
predeclared as one or the other, during compilation.

However, linking also needs to happen. And as it turns out, [whether a
thing is a class or struct is part of the MSVC name mangling
spec](https://en.wikiversity.org/wiki/Visual_C%2B%2B_name_mangling).
Hence, we'd end up with e.g. this function (which references
`SymbolInfo`) as being defined in it's object file as the following
symbol:
>
`lookupSymbolsByLocation@SymbolTable@textplan@substrait@io@@qeba?AV?$vector@PEBUSymbolInfo@textplan@substrait@io@@v?$allocator@PEBUSymbolInfo@textplan@substrait@io@@@std@@@std@@AEBVLocation@234@@Z`

However, things which reference `SymbolInfo`, but which may have pulled
in the incorrect predeclaration prior, will reference it as an incorrect
symbol:
>
`lookupSymbolsByLocation@SymbolTable@textplan@substrait@io@@qeba?AV?$vector@PEBVSymbolInfo@textplan@substrait@io@@v?$allocator@PEBVSymbolInfo@textplan@substrait@io@@@std@@@std@@AEBVLocation@234@@z
...`

e.g. notice the difference between `PEBUSymbolInfo` and
`PEBVSymbolInfo`. Enqueue strange linking issues galore.
  • Loading branch information
mortbopet committed Sep 7, 2024
1 parent 94ff855 commit 3ea39df
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/substrait/textplan/StructuredSymbolData.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

namespace io::substrait::textplan {

class SymbolInfo;
struct SymbolInfo;

// Used by the PlanRelation and Relation concepts to track connectivity.
struct RelationData {
Expand Down

0 comments on commit 3ea39df

Please sign in to comment.