From 163f2d2d7066b3a44d1ffbd82e9cfa0be103468e Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Thu, 5 Sep 2024 15:40:50 +0200 Subject: [PATCH 1/2] Don't define member functions inside a nested namespace This trips up MSVC, in that member functions aren't properly included in the object files of the various translation units. Generally, this should be fixed by having a `using` declaration in a .cpp file. From my experience, a general good practice is to never nest definitions in namespaces, unless that is only visible from within a translation unit (e.g. a `detail` namespace inside a .cpp file). This doesn't fix all of the places in the code where this pattern is used - only the (narrow) compile path that i've been excercising. Someone should probably go through the rest of the codebase and apply this - will be needed if/when a windows CI pipeline is added. --- src/substrait/textplan/SymbolTable.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/substrait/textplan/SymbolTable.cpp b/src/substrait/textplan/SymbolTable.cpp index 66d25579..78a1e82b 100644 --- a/src/substrait/textplan/SymbolTable.cpp +++ b/src/substrait/textplan/SymbolTable.cpp @@ -12,7 +12,7 @@ #include "substrait/textplan/Location.h" #include "substrait/textplan/StructuredSymbolData.h" -namespace io::substrait::textplan { +using namespace io::substrait::textplan; const std::string& symbolTypeName(SymbolType type) { static std::vector names = { @@ -60,13 +60,17 @@ const SymbolInfo SymbolInfo::kUnknown = { std::nullopt, std::nullopt}; -bool operator==(const SymbolInfo& left, const SymbolInfo& right) { +bool io::substrait::textplan::operator==( + const SymbolInfo& left, + const SymbolInfo& right) { return (left.name == right.name) && (left.sourceLocation == right.sourceLocation) && (left.type == right.type); } -bool operator!=(const SymbolInfo& left, const SymbolInfo& right) { +bool io::substrait::textplan::operator!=( + const SymbolInfo& left, + const SymbolInfo& right) { return !(left == right); } @@ -288,5 +292,3 @@ std::string SymbolTable::toDebugString() const { } return result.str(); } - -} // namespace io::substrait::textplan From 1e1e508f098d97d0827d4b538b0b670a8c429321 Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Thu, 5 Sep 2024 17:39:16 +0200 Subject: [PATCH 2/2] explicit namespace for free function --- src/substrait/textplan/SymbolTable.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/substrait/textplan/SymbolTable.cpp b/src/substrait/textplan/SymbolTable.cpp index 78a1e82b..568a531e 100644 --- a/src/substrait/textplan/SymbolTable.cpp +++ b/src/substrait/textplan/SymbolTable.cpp @@ -14,7 +14,7 @@ using namespace io::substrait::textplan; -const std::string& symbolTypeName(SymbolType type) { +const std::string& io::substrait::textplan::symbolTypeName(SymbolType type) { static std::vector names = { "kExtensionSpace", "kFunction",