Skip to content

fix: Enable DESTRUCTOR_CLOSES_FILE flag#1075

Merged
znelson merged 1 commit intomasterfrom
fix/destructure-closes-file
Mar 3, 2026
Merged

fix: Enable DESTRUCTOR_CLOSES_FILE flag#1075
znelson merged 1 commit intomasterfrom
fix/destructure-closes-file

Conversation

@daveallie
Copy link
Member

@daveallie daveallie commented Feb 22, 2026

Summary

  • Enable DESTRUCTOR_CLOSES_FILE flag
  • We're never intending to not close files, so if we accidentally leave them open as they're destructured, this will help close them.

Additional Context

Looks to use about 5K of flash.

RAM:   [===       ]  31.5% (used 103100 bytes from 327680 bytes)
Flash: [=======   ]  68.9% (used 4513220 bytes from 6553600 bytes)

RAM:   [===       ]  31.5% (used 103100 bytes from 327680 bytes)
Flash: [=======   ]  68.9% (used 4518498 bytes from 6553600 bytes)

AI Usage

While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.

Did you use AI tools to help write this code? No

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 22, 2026 •
edited
Loading

No actionable comments were generated in the recent review. 🎉

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3cb60aa and d05f6ff.

📒 Files selected for processing (1)
  • platformio.ini
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: cppcheck
  • GitHub Check: build
🔇 Additional comments (2)
platformio.ini (2)

87-93: AI summary inconsistency: -UENABLE_SERIAL_LOG was not removed.

The AI-generated summary claims that -UENABLE_SERIAL_LOG was removed from slim builds, but the code shows it's still present at line 93. The actual change in this PR only adds the DESTRUCTOR_CLOSES_FILE flag at line 28.

28-28: No additional action required—the codebase already implements safe file handling practices.

The DESTRUCTOR_CLOSES_FILE flag is a valid SdFat configuration macro that enables automatic close() in file object destructors. Verification confirms the codebase is well-prepared for this change:

  • All file objects use explicit .close() calls (50+ instances across the codebase), following SdFat best practices regardless of this flag.
  • File operations consistently follow a safe pattern: local scope objects with explicit cleanup before scope exit.
  • The single global FsFile object (wsUploadFile in CrossPointWebServer.cpp) is also explicitly closed, mitigating concerns about global destructor behavior on embedded systems.

The flag acts as a safety net for any edge cases where explicit close is missed, and the existing code discipline means the codebase is already compatible with this change.

📝 Walkthrough

Walkthrough

Modified build configuration flags in platformio.ini: added -DDESTRUCTOR_CLOSES_FILE=1 to base build flags and removed the -UENABLE_SERIAL_LOG undefine from slim builds, enabling serial logging in slim builds.

Changes

Cohort / File(s) Summary
Build Configuration
platformio.ini		 Added destructor file-closing flag to base build configuration; removed serial logging disable flag from slim builds, re-enabling serial logging.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the primary change in the pull request: enabling the DESTRUCTOR_CLOSES_FILE flag as documented in the raw summary and PR objectives.
Description check ✅ Passed The description directly relates to the changeset, explaining why the DESTRUCTOR_CLOSES_FILE flag is being enabled and providing context about files being accidentally left open.

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

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

znelson
znelson approved these changes Feb 23, 2026
View reviewed changes
Copy link
Contributor

@znelson znelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we clean up some .close() calls now?

@znelson znelson requested a review from ngxson February 23, 2026 21:01
@znelson znelson merged commit 4388bf8 into master Mar 3, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@znelson znelson znelson approved these changes

@jonasdiemer jonasdiemer jonasdiemer approved these changes

@ngxson ngxson Awaiting requested review from ngxson

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@daveallie @znelson @jonasdiemer