hiero-ledger / hiero-ledger/hiero-sdk-cpp
[Good First Issue]: Improve comments on Windows-specific include-ordering workarounds
- Dominant language
- C++
- Stars
- 42
- Forks
- 108
- Avg merge
- 11h 45m
- Merged PRs (30d)
- 2
Description
### 🆕🐥 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*:
```cpp
// Windows build requires this to be included first for some reason.
#include // 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: `` 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 `` 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:**
```cpp
// Windows build requires this to be included first for some reason.
```
**Replace with:**
```cpp
// Must be included before any header that pulls in , whose macros (e.g. GetMessage) collide with identifiers in the generated protobuf code.
```
- [ ] In `src/sdk/main/src/FeeEstimateQuery.cc`, replace line 2:
**Find:**
```cpp
// Windows build requires this to be included first for some reason.
```
**Replace with:**
```cpp
// Must be included before any header that pulls in , 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:**
```cpp
#include // This is needed for Windows to build for some reason.
```
**Replace with:**
```cpp
#include // Must be included before any header that pulls in , whose macros (e.g. GetMessage) collide with identifiers in the generated protobuf code.
```
- [ ] In `src/tck/src/TckServer.cc`, replace line 2:
**Find:**
```cpp
// Windows build requires this to be included first for some reason.
```
**Replace with:**
```cpp
// Must be included before any header that pulls in , 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 #`.
### ✔️ 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](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/workflow.md) for step-by-step workflow guidance.
Read [README.md](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/README.md) for setup instructions.
❗ Pull requests **cannot be merged** without `S` and `s` signed commits.
See the [Signing Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/signing.md).
### 🤔 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](https://discord.com/channels/905194001349627914/1337424839761465364)
Whether you need help finding the right file, understanding existing code, or confirming your approach — we're happy to help.
Contributor guide
Assessment
This issue has not been assessed yet.