Skip to content

3600: AVRO-4221: [C++] Allow using symbol visibility annotations also on non-Windows platforms #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martin-augment wants to merge 1 commit into
base: main
Choose a base branch
from
Open

3600: AVRO-4221: [C++] Allow using symbol visibility annotations also on non-Windows platforms #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lang/c++/impl/json/JsonDom.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

namespace avro {

class AVRO_DECL InputStream;
class InputStream;

namespace json {
class Entity;
Expand Down Expand Up @@ -59,7 +59,7 @@ enum class EntityType {
Obj
};

const char *typeToString(EntityType t);
AVRO_DECL const char *typeToString(EntityType t);
Copy link
Copy Markdown

@augmentcode augmentcode bot Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeToString is now declared with AVRO_DECL, but its definition in impl/json/JsonDom.cc appears to be compiled without including JsonDom.hh, so the visibility/export annotation may not be applied to the definition in the library TU. This can matter when building the shared library with hidden-by-default visibility, where consumers would then fail to link against typeToString.

🤖 Was this useful? React with 👍 or 👎

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Dec 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:annoying; category:bug; feedback:The Augment AI reviewer is not correct! JsonDom.cc includes JsonDom.hh at https://github.com/apache/avro/blob/52938d00107626aaedbafb2ea9e9fb50f3b95138/lang/c%2B%2B/impl/json/JsonDom.cc#L19


inline std::ostream &operator<<(std::ostream &os, EntityType et) {
return os << typeToString(et);
Expand Down
8 changes: 4 additions & 4 deletions lang/c++/include/avro/Compiler.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@

namespace avro {

class AVRO_DECL InputStream;
class InputStream;

/// This class is used to implement an avro spec parser using a flex/bison
/// compiler. In order for the lexer to be reentrant, this class provides a
/// lexer object for each parse. The bison parser also uses this class to
/// build up an avro parse tree as the avro spec is parsed.

class AVRO_DECL Name;
class AVRO_DECL ValidSchema;
class Name;
Copy link
Copy Markdown

@augmentcode augmentcode bot Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing AVRO_DECL from these forward declarations (e.g., Name/ValidSchema/InputStream) can create inconsistent DLL linkage/visibility if a TU later includes the defining headers where these types are declared as class AVRO_DECL .... On GCC/Clang in particular, visibility attributes are commonly only honored on the first declaration, so this may accidentally drop the intended export.

🤖 Was this useful? React with 👍 or 👎

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:annoying; category:bug; feedback:The Augment AI reviewer is not correct! The AVRO_DECL macro should be used where the class is defined, i.e. for Name this is Node.hh, for ValidSchema this is ValidSchema.hh, and for InputStream this is Stream.hh.

class ValidSchema;

/// Given a stream containing a JSON schema, compiles the schema to a
/// ValidSchema object. Throws if the schema cannot be compiled to a valid
Expand All @@ -60,7 +60,7 @@ AVRO_DECL ValidSchema compileJsonSchemaFromString(const std::string &input);

AVRO_DECL ValidSchema compileJsonSchemaFromFile(const char *filename);

AVRO_DECL ValidSchema compileJsonSchemaWithNamedReferences(std::istream &is,
AVRO_DECL ValidSchema compileJsonSchemaWithNamedReferences(std::istream &is,
const std::map<Name, ValidSchema> &namedReferences);

} // namespace avro
Expand Down
20 changes: 16 additions & 4 deletions lang/c++/include/avro/Config.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,29 @@

// Windows DLL support

#ifdef _WIN32
#ifdef _MSC_VER
#pragma warning(disable : 4275 4251)
#endif // _MSC_VER

#if defined _WIN32 || defined __CYGWIN__
#define AVRO_DLL_EXPORT __declspec(dllexport)
#define AVRO_DLL_IMPORT __declspec(dllimport)
#define AVRO_DLL_HIDDEN
#else
#define AVRO_DLL_EXPORT [[gnu::visibility("default")]]
#define AVRO_DLL_IMPORT [[gnu::visibility("default")]]
#define AVRO_DLL_HIDDEN [[gnu::visibility("hidden")]]
#endif // _WIN32 || __CYGWIN__
Comment on lines +28 to +36
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect the actual Config.hh file to see the current state
cat "lang/c++/include/avro/Config.hh" | head -50

Repository: martin-augment/avro

Length of output: 1612

🏁 Script executed:

#!/bin/bash
# Search for C++ standard requirements
fd -e cmake . | xargs grep -n "CMAKE_CXX_STANDARD\|CXX_STANDARD" 2>/dev/null || echo "No CMake C++ standard found"

echo "---"

# Check README for compiler requirements
fd -e md README . 2>/dev/null | head -5 | xargs -I {} cat {} | grep -i "c++\|compiler\|require" -A 2 -B 2 2>/dev/null || echo "No README compiler info found"

Repository: martin-augment/avro

Length of output: 2895

Ensure C++17 minimum version is explicitly enforced in the build configuration.

The code uses the newer standard C and C++ syntax using '[[...]]' with the 'gnu::' prefix, which requires C++17 or later. The code properly handles Windows platforms with conditional compilation, but the CMake configuration lacks an explicit CMAKE_CXX_STANDARD requirement. Add set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard" FORCE) to CMakeLists.txt to ensure all builds target C++17+, preventing compilation failures on projects using C++14 or earlier.

🤖 Prompt for AI Agents 
In lang/c++/include/avro/Config.hh around lines 28 to 36, the file uses
C++17-specific attributes ([[gnu::visibility(...)]]), but the project CMake does
not enforce a minimum C++ standard; update the top-level CMakeLists.txt to
require C++17 by adding set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard"
FORCE) (or equivalently use target_compile_features/target_compile_options to
require cxx_std_17) so all targets build with at least C++17 and prevent
compilation failures on older default standards.

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:bug; feedback:The CodeRabbit AI reviewer is correct! The AvroCpp project requires C++17 - https://github.com/apache/avro/blob/99c7379ce62d002daca4d94b975afabc725df024/lang/c%2B%2B/CMakeLists.txt#L24 and everything is OK.


#if defined(AVRO_DYN_LINK)
#ifdef AVRO_DYN_LINK
#ifdef AVRO_SOURCE
#define AVRO_DECL __declspec(dllexport)
#define AVRO_DECL AVRO_DLL_EXPORT
#else
#define AVRO_DECL __declspec(dllimport)
#define AVRO_DECL AVRO_DLL_IMPORT
#endif // AVRO_SOURCE
#endif // AVRO_DYN_LINK

#ifdef _WIN32
#include <intsafe.h>
using ssize_t = SSIZE_T;
#endif // _WIN32
Expand Down