Skip to content

fix: improve windows include comments#1678

Open
mohityadav8 wants to merge 1 commit into
hiero-ledger:mainfrom
mohityadav8:fix/improve-windows-include-comments
Open

fix: improve windows include comments#1678
mohityadav8 wants to merge 1 commit into
hiero-ledger:mainfrom
mohityadav8:fix/improve-windows-include-comments

Conversation

@mohityadav8

@mohityadav8 mohityadav8 commented Jun 14, 2026

Copy link
Copy Markdown

Description:

Improve Windows-specific include-ordering comments by replacing vague explanations with clear descriptions of the underlying macro-collision issue caused by <windows.h>.

  • Update comments in AccountId.cc
  • Update comments in FeeEstimateQuery.cc
  • Update comments in BaseNode.h
  • Update comments in TckServer.cc
  • Clarify why protobuf headers must be included before headers that may pull in <windows.h>

Related issue(s):

Fixes #1632

Notes for reviewer:

This PR only updates code comments and does not introduce any functional or behavioral changes. The new comments explain the known Windows macro-collision issue (GetMessage, SendMessage, etc.) that can interfere with protobuf-generated code and justify the include-ordering workaround.

Checklist

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

Summary by CodeRabbit

  • Documentation
    • Updated comments across multiple files to clarify Windows build include-order requirements, preventing macro collisions with generated protobuf code.

Signed-off-by: Mohit <mohityadav8@users.noreply.github.com>
@mohityadav8 mohityadav8 requested review from a team as code owners June 14, 2026 13:02
@mohityadav8 mohityadav8 requested a review from rwalworth June 14, 2026 13:02
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d2bad93b-37b1-4e47-953d-b81f9d3086a0

📥 Commits

Reviewing files that changed from the base of the PR and between a898153 and b43f849.

📒 Files selected for processing (4)
  • src/sdk/main/include/impl/BaseNode.h
  • src/sdk/main/src/AccountId.cc
  • src/sdk/main/src/FeeEstimateQuery.cc
  • src/tck/src/TckServer.cc

📝 Walkthrough

Walkthrough

Four file-header comments describing a Windows-specific include-order workaround are updated. The vague phrase "for some reason" is replaced with a precise explanation that the proto header must precede any header that pulls in <windows.h>, whose macros collide with identifiers in the generated protobuf code.

Changes

Windows include-order comment clarification

Layer / File(s) Summary
Updated include-order 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
Replaces the one-line vague comment ("for some reason") on the out-of-order proto #include with a descriptive sentence naming <windows.h> macro pollution (e.g. GetMessage) as the cause. No #include directives, // NOLINT suffixes, or executable code are changed.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

🐇 A comment once murky, now clear as the dawn,
"For some reason" removed, the confusion is gone.
GetMessage macros from Windows would bite,
So proto comes first — now we spell out the plight.
Hop hop, little fix, you've shed so much light! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: improving Windows-specific include-order comments across four files.
Linked Issues check ✅ Passed The PR fully implements all requirements from issue #1632: all four files have their vague comments replaced with the exact replacement text specified.
Out of Scope Changes check ✅ Passed All changes are strictly comment updates in the four specified files with no code logic modifications, fully aligned with the linked issue scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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 @mohityadav8 👋 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 Jun 14, 2026
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