Skip to content

docs: clarify Windows protobuf include-order comments#1659

Open
Sri-Varshith wants to merge 1 commit into
hiero-ledger:mainfrom
Sri-Varshith:docs/windows-include-comment-fix
Open

docs: clarify Windows protobuf include-order comments#1659
Sri-Varshith wants to merge 1 commit into
hiero-ledger:mainfrom
Sri-Varshith:docs/windows-include-comment-fix

Conversation

@Sri-Varshith

@Sri-Varshith Sri-Varshith commented May 20, 2026

Copy link
Copy Markdown

Description:

Clarify Windows include-order comments related to protobuf and windows.h macro collisions.

  • Replace vague Windows include-order comments with descriptive explanations
  • Preserve existing include ordering and behavior
  • No functional code changes

Related issue(s):

Fixes #1632

Notes for reviewer:

Comment-only change. No behavior changes.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Summary by CodeRabbit

  • Documentation
    • Updated internal code documentation to clarify include ordering requirements, improving SDK compilation stability on Windows builds.

Review Change Stack

Signed-off-by: Sri Varshith <nvarshith.2007@gmail.com>
@Sri-Varshith Sri-Varshith requested review from a team as code owners May 20, 2026 10:52
@Sri-Varshith Sri-Varshith requested a review from gsstoykov May 20, 2026 10:52
@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Four files include comments clarifying why a protobuf header must be included before any header that pulls in <windows.h>: macro collisions in generated protobuf code. The comments replace vague explanations with factual descriptions of the include-ordering requirement and its root cause.

Changes

Windows Include-Order Documentation

Layer / File(s) Summary
Clarify protobuf/Windows include-ordering workaround comments
src/sdk/main/include/impl/BaseNode.h, src/sdk/main/src/AccountId.cc, src/sdk/main/src/FeeEstimateQuery.cc, src/tck/src/TckServer.cc
Comments are updated in four files to specify that protobuf-generated headers must precede any header pulling in <windows.h> to prevent macro collisions (e.g., GetMessage) with generated protobuf identifiers. No code logic or declarations changed.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

📋 A rabbit hops through comments clear,
Explaining why includes belong right here,
No macros clash, no chaos flows,
When protobuf headers come first—and that's how it goes! 🐰

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: clarifying vague Windows protobuf include-order comments across four files.
Linked Issues check ✅ Passed All four files have their comments replaced with the exact descriptive text specified in issue #1632, explaining protobuf macro collisions with Windows headers.
Out of Scope Changes check ✅ Passed All changes are strictly in-scope: only comment updates in the four specified files with no modifications to include lines, code logic, or surrounding structure.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown

Hey @Sri-Varshith 👋 thanks for the PR!
I'm your friendly PR Helper Bot 🤖 and I'll be riding shotgun on this one, keeping track of your PR's status to help you get it approved and merged.

This comment updates automatically as you push changes -- think of it as your PR's live scoreboard!
Here's the latest:


PR Checks

DCO Sign-off -- All commits have valid sign-offs. Nice work!


GPG Signature -- All commits have verified GPG signatures. Locked and loaded!


Merge Conflicts -- No merge conflicts detected. Smooth sailing!


Issue Link -- Linked to #1632 (assigned to you).


🎉 All checks passed! Your PR is ready for review. Great job!

@github-actions github-actions Bot added the status: needs review The pull request is ready for maintainer review label May 20, 2026
@Sri-Varshith

Copy link
Copy Markdown
Author

@gsstoykov could you pls review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs review The pull request is ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant