Skip to content

[Good First Issue]: Improve comments on Windows-specific include-ordering workarounds #1632

Description

@rwalworth

🆕🐥 First-Time Friendly

This issue is especially welcoming for people who are new to contributing to the Hiero C++ SDK.

We know that opening your first pull request can feel like a big step. Issues labeled Good First Issue are designed to make that experience easier, clearer, and more comfortable.

No prior knowledge of Hiero, Hedera, or distributed ledger technology is required - just a basic familiarity with C++ and Git is more than enough to get started.

Important

📋 About Good First Issues

Good First Issues are designed to make getting started as smooth and stress-free as possible.

They usually focus on:

  • Small, clearly scoped changes
  • Straightforward updates to existing code or docs
  • Simple refactors or clarity improvements

Other kinds of contributions — like larger features, deeper technical changes, or design-focused work — are just as valuable and often use the beginner, intermediate, or advanced labels.

👾 Description of the Task

Four files in the codebase include a header out of normal alphabetical order to work around a Windows-only build failure. Each location has a one-line comment that explains the what but not the why:

// Windows build requires this to be included first for some reason.
#include <services/basic_types.pb.h> // NOLINT

"for some reason" leaves the next reader without enough context to know whether the workaround is still needed, what symptom it prevents, or where else they might run into the same issue.

The underlying cause is well-understood: <windows.h> defines preprocessor macros for several common identifiers (GetMessage, SendMessage, etc.) that mangle into *A/*W variants. When any header on the include path pulls in <windows.h> before the protobuf-generated headers are parsed, those macros collide with identically-named identifiers in the generated code and the build fails. Forcing a proto header to be included first parses the proto translation unit before any Windows macro pollution can leak in.

This task replaces the vague comment in each of the four locations with a single, more descriptive sentence — no code changes, no behavior changes.

Files to change:

src/sdk/main/src/AccountId.cc
src/sdk/main/src/FeeEstimateQuery.cc
src/sdk/main/include/impl/BaseNode.h
src/tck/src/TckServer.cc

💡 Proposed Approach

Replace the existing one-line comment in each of the four files with the exact replacement text shown below. Leave every other line — including the #include directive itself, the // NOLINT suffix where present, and the surrounding blank lines — untouched.

👩‍💻 Implementation Steps

  • In src/sdk/main/src/AccountId.cc, replace line 2:

    Find:

    // Windows build requires this to be included first for some reason.

    Replace with:

    // Must be included before any header that pulls in <windows.h>, whose macros (e.g. GetMessage) collide with identifiers in the generated protobuf code.
  • In src/sdk/main/src/FeeEstimateQuery.cc, replace line 2:

    Find:

    // Windows build requires this to be included first for some reason.

    Replace with:

    // Must be included before any header that pulls in <windows.h>, whose macros (e.g. GetMessage) collide with identifiers in the generated protobuf code.
  • In src/sdk/main/include/impl/BaseNode.h, replace the trailing comment on line 5:

    Find:

    #include <services/basic_types.pb.h> // This is needed for Windows to build for some reason.

    Replace with:

    #include <services/basic_types.pb.h> // Must be included before any header that pulls in <windows.h>, whose macros (e.g. GetMessage) collide with identifiers in the generated protobuf code.
  • In src/tck/src/TckServer.cc, replace line 2:

    Find:

    // Windows build requires this to be included first for some reason.

    Replace with:

    // Must be included before any header that pulls in <windows.h>, whose macros (e.g. GetMessage) collide with identifiers reached transitively from this header.
  • Verify the local build still succeeds on your platform:
    bash cmake --preset linux-x64-debug -DBUILD_TESTS=ON -DBUILD_TCK_TESTS=ON cmake --build -j 6 --preset linux-x64-debug

  • Open a pull request that references this issue with Fixes #<this-issue-number>.

✔️ Acceptance Criteria

  • Each of the four listed files has its Windows include-order comment replaced with the exact text shown above.
  • No #include lines, // NOLINT suffixes, or surrounding code are modified.
  • The Linux, macOS, and Windows builds continue to pass in CI.

📋 Step-by-Step Contribution Guide

To help keep contributions consistent and easy to review, we recommend following these steps:

  • Comment /assign to request the issue
  • Wait for assignment
  • Fork the repository and create a branch
  • Set up the project using the instructions in README.md
  • Make the requested changes
  • Sign each commit using -s -S
  • Push your branch and open a pull request

Read Workflow Guide for step-by-step workflow guidance.
Read README.md for setup instructions.

❗ Pull requests cannot be merged without S and s signed commits.
See the Signing Guide.

🤔 Additional Information

If you have questions while working on this issue, feel free to ask!

You can reach the community and maintainers here: Hiero-SDK-C++ Discord

Whether you need help finding the right file, understanding existing code, or confirming your approach — we're happy to help.

Metadata

Metadata

Assignees

Labels

priority: lowNon-urgent tasks, nice-to-have improvements, or minor issuesscope: docsRelated to READMEs, guides, API documentation, or code commentsskill: good first issueSimple, well-scoped tasks ideal for someone new to the repository or open sourcestatus: in progressA contributor is actively working on this issue

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions