fix(C++): remove redundant inline keyword and fix typos

[SYaoJun]

Reason for this PR

• member functions defined within a class declaration are implicitly inline — explicit inline specifier here is redundant and unnecessary. (Ref: https://eel.is/c++draft/class.mfct#1)

What changes are included in this PR?

• Adding inline to such functions does not change behavior, but adds unnecessary verbosity to the code. For consistency and readability, we remove redundant inline specifiers from in-class defined member functions in this PR.

Are these changes tested?

yes

Are there any user-facing changes?

no

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.08%. Comparing base (95ac39b) to head (fff5969).

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #858      +/-   ##
============================================
- Coverage     80.21%   80.08%   -0.14%     
  Complexity      615      615              
============================================
  Files            93       93              
  Lines         10255    10186      -69     
  Branches       1049     1043       -6     
============================================
- Hits           8226     8157      -69     
  Misses         1789     1789              
  Partials        240      240              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SYaoJun]

I found a CMake bug in the benchmark after updating the code. I’ll fix it as part of this update.

[100%] Linking CXX executable graph_info_benchmark
/usr/bin/ld: CMakeFiles/graph_info_benchmark.dir/benchmarks/graph_info_benchmark.cc.o: undefined reference to symbol '_ZN7parquet5arrow8OpenFileESt10shared_ptrIN5arrow2io16RandomAccessFileEEPNS2_10MemoryPoolEPSt10unique_ptrINS0_10FileReaderESt14default_deleteIS9_EE'
/usr/bin/ld: /lib/x86_64-linux-gnu/libparquet.so.1700: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
make[2]: *** [CMakeFiles/graph_info_benchmark.dir/build.make:103: graph_info_benchmark] Error 1
make[1]: *** [CMakeFiles/Makefile2:2024: CMakeFiles/graph_info_benchmark.dir/all] Error 2
make: *** [Makefile:146: all] Error 2
``` `

[Copilot]

Pull request overview

This pull request aims to improve code quality by removing redundant inline keywords from member functions defined within class declarations and fixing several typos throughout the C++ codebase. According to the C++ standard (class.mfct#1), member functions defined within a class declaration are implicitly inline, making explicit inline specifiers unnecessary and verbose.

Changes:

• Removed redundant inline keywords from in-class member function definitions across multiple header files
• Fixed typos in comments ("itetate" → "iterate") and variable names ("indice" → "index")
• Fixed typos in CHANGELOG.md ("increse" → "increase", "effeciency" → "efficiency")
• Added division by zero check in edges_builder.h
• Updated CMakeLists.txt benchmark linking configuration for platform-specific library handling

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
cpp/src/graphar/version_parser.h	Removed inline from copy assignment operator and CheckType method
cpp/src/graphar/util.h	Removed inline from namespace-level static utility functions and struct member
cpp/src/graphar/types.h	Removed inline from copy assignment operator and namespace-level conversion functions
cpp/src/graphar/status.h	Removed inline from constructors, assignment operators, and OK() method
cpp/src/graphar/result.h	Removed inline from template helper functions
cpp/src/graphar/label.h	Removed inline from SetBitmap utility function
cpp/src/graphar/high-level/vertices_builder.h	Removed inline from Vertex and VerticesBuilder member functions
cpp/src/graphar/high-level/graph_reader.h	Removed inline from Vertex and Edge accessor methods
cpp/src/graphar/high-level/graph_reader.cc	Fixed typo in comment: "itetate" → "iterate"
cpp/src/graphar/high-level/edges_builder.h	Removed inline from Edge and EdgesBuilder methods; added division by zero check
cpp/src/graphar/graph_info.h	Removed inline from PropertyGroup and AdjacentList accessor methods
cpp/src/graphar/expression.h	Removed inline from protected method and helper functions
cpp/src/graphar/arrow/chunk_writer.h	Removed inline from validation level accessor methods
cpp/src/graphar/arrow/chunk_writer.cc	Renamed variable from "indice" to "index" throughout
cpp/CMakeLists.txt	Updated benchmark linking to use platform-specific library targets
CHANGELOG.md	Fixed typos: "increse" → "increase", "effeciency" → "efficiency"

Comments suppressed due to low confidence (2)

cpp/src/graphar/types.h:272

• The removal of inline from these static namespace-level functions (lines 198, 207, 217, 233, 246, 255, 268) doesn't align with the PR description's rationale. The cited C++ standard section (class.mfct#1) only applies to member functions defined within class declarations. These are static functions at namespace scope, where removing inline causes each translation unit to get its own copy of the function (code bloat), rather than sharing a single definition. Consider keeping inline for these functions.

static const char* AdjListTypeToString(AdjListType adj_list_type) {
  static const std::map<AdjListType, const char*> adj_list2string{
      {AdjListType::unordered_by_source, "unordered_by_source"},
      {AdjListType::unordered_by_dest, "unordered_by_dest"},
      {AdjListType::ordered_by_source, "ordered_by_source"},
      {AdjListType::ordered_by_dest, "ordered_by_dest"}};
  return adj_list2string.at(adj_list_type);
}

static AdjListType OrderedAlignedToAdjListType(bool ordered,
                                               const std::string& aligned) {
  if (ordered) {
    return aligned == "src" ? AdjListType::ordered_by_source
                            : AdjListType::ordered_by_dest;
  }
  return aligned == "src" ? AdjListType::unordered_by_source
                          : AdjListType::unordered_by_dest;
}

static std::pair<bool, std::string> AdjListTypeToOrderedAligned(
    AdjListType adj_list_type) {
  switch (adj_list_type) {
  case AdjListType::unordered_by_source:
    return std::make_pair(false, "src");
  case AdjListType::unordered_by_dest:
    return std::make_pair(false, "dst");
  case AdjListType::ordered_by_source:
    return std::make_pair(true, "src");
  case AdjListType::ordered_by_dest:
    return std::make_pair(true, "dst");
  default:
    return std::make_pair(false, "dst");
  }
}

static FileType StringToFileType(const std::string& str) {
  static const std::map<std::string, FileType> str2file_type{
      {"csv", FileType::CSV},
      {"json", FileType::JSON},
      {"parquet", FileType::PARQUET},
      {"orc", FileType::ORC}};
  try {
    return str2file_type.at(str.c_str());
  } catch (const std::exception& e) {
    throw std::runtime_error("KeyError: " + str);
  }
}

static const char* FileTypeToString(FileType file_type) {
  static const std::map<FileType, const char*> file_type2string{
      {FileType::CSV, "csv"},
      {FileType::JSON, "json"},
      {FileType::PARQUET, "parquet"},
      {FileType::ORC, "orc"}};
  return file_type2string.at(file_type);
}

static Cardinality StringToCardinality(const std::string& str) {
  static const std::map<std::string, Cardinality> str2cardinality{
      {"single", Cardinality::SINGLE},
      {"list", Cardinality::LIST},
      {"set", Cardinality::SET},
  };
  try {
    return str2cardinality.at(str.c_str());
  } catch (const std::exception& e) {
    throw std::runtime_error("KeyError: " + str);
  }
}

static const char* CardinalityToString(Cardinality cardinality) {
  static const std::map<Cardinality, const char*> cardinality2string{
      {Cardinality::SINGLE, "single"},
      {Cardinality::LIST, "list"},
      {Cardinality::SET, "set"},

cpp/src/graphar/util.h:274

• The removal of inline from these static functions at namespace scope (lines 206, 217, 241, 262 in util.h) is different from removing it from in-class member function definitions. The C++ standard section cited in the PR description (class.mfct#1) only applies to member functions defined within class declarations, not to namespace-level static functions. While static functions have internal linkage and won't cause ODR violations, removing inline here means each translation unit that includes util.h gets its own copy of these functions, leading to code bloat. Consider keeping inline for these namespace-level functions or moving them to util.cc.

static IdType IndexPairToGlobalChunkIndex(
    const std::vector<IdType>& edge_chunk_nums, IdType vertex_chunk_index,
    IdType edge_chunk_index) {
  IdType global_edge_chunk_index = 0;
  for (IdType i = 0; i < vertex_chunk_index; ++i) {
    global_edge_chunk_index += edge_chunk_nums[i];
  }
  return global_edge_chunk_index + edge_chunk_index;
}

// covert edge global chunk index to <vertex_chunk_index, edge_chunk_index>
static std::pair<IdType, IdType> GlobalChunkIndexToIndexPair(
    const std::vector<IdType>& edge_chunk_nums, IdType global_index) {
  std::pair<IdType, IdType> index_pair(0, 0);
  for (size_t i = 0; i < edge_chunk_nums.size(); ++i) {
    if (global_index < edge_chunk_nums[i]) {
      index_pair.first = static_cast<IdType>(i);
      index_pair.second = global_index;
      break;
    }
    global_index -= edge_chunk_nums[i];
  }
  return index_pair;
}

std::shared_ptr<arrow::ChunkedArray> GetArrowColumnByName(
    std::shared_ptr<arrow::Table> const& table, const std::string& name);

std::shared_ptr<arrow::Array> GetArrowArrayByChunkIndex(
    std::shared_ptr<arrow::ChunkedArray> const& chunk_array,
    int64_t chunk_index);

Result<const void*> GetArrowArrayData(
    std::shared_ptr<arrow::Array> const& array);

static std::string ConcatStringWithDelimiter(
    const std::vector<std::string>& str_vec, const std::string& delimiter) {
  return std::accumulate(
      std::begin(str_vec), std::end(str_vec), std::string(),
      [&delimiter](const std::string& ss, const std::string& s) {
        return ss.empty() ? s : ss + delimiter + s;
      });
}

template <typename T>
struct ValueGetter {
  static T Value(const void* data, int64_t offset) {
    return reinterpret_cast<const T*>(data)[offset];
  }
};

template <>
struct ValueGetter<std::string> {
  static std::string Value(const void* data, int64_t offset);
};

static arrow::Status OpenParquetArrowReader(
    const std::string& file_path, arrow::MemoryPool* pool,
    std::unique_ptr<parquet::arrow::FileReader>* parquet_reader) {
  std::shared_ptr<arrow::io::RandomAccessFile> input;
  ARROW_ASSIGN_OR_RAISE(input, arrow::io::ReadableFile::Open(file_path));
#if defined(ARROW_VERSION) && ARROW_VERSION <= 20000000
  ARROW_RETURN_NOT_OK(parquet::arrow::OpenFile(input, pool, parquet_reader));
#else
  ARROW_ASSIGN_OR_RAISE(auto reader, parquet::arrow::OpenFile(input, pool));
  *parquet_reader = std::move(reader);
#endif
  return arrow::Status::OK();
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The removal of inline from these static namespace-level helper functions (lines 292-357) doesn't align with the PR description's rationale. The cited C++ standard section (class.mfct#1) only applies to member functions defined within class declarations. These are static functions at namespace scope in a header file, where removing inline causes each translation unit to get its own copy of the functions. Consider keeping inline for these functions.

[Copilot]

The changes to the benchmark linking configuration don't align with the stated PR purpose of "removing redundant inline keywords and fixing typos." This appears to be an unrelated build system change that adjusts how Arrow and Parquet libraries are linked for benchmarks on different platforms (using namespaced targets on Apple vs. plain library names on other platforms). This change should either be documented in the PR description or moved to a separate PR focused on build system improvements.

[Copilot]

This division by zero check is a bug fix that doesn't align with the stated PR purpose of "removing redundant inline keywords and fixing typos." While this is a good defensive programming practice to prevent potential crashes, it should be documented in the PR description or moved to a separate bug fix PR. The fix prevents division by zero when vertex_chunk_size_ is 0, but it's unclear if returning 0 is the correct behavior in this case or if it should be an error.