Adopt a different convention re order of includes#787
Merged
Conversation
Fixes #775 and, hopefully, heads off future Alpine issues of this nature. Exactly why I was ordering includes as I was has been lost in the sands of time. But this seems more conventional: system → cpp11 → other vendored (libxls, rapidxml) → project Within the group, alphabetical. Blank lines between. For a .cpp file, the corresponding .h sits at the top. Why system before libxls? we want standard headers like <stdint.h> processed in the global namespace, not inside xls. This is #775. Why cpp11 before libxls? libxls transitively pulls in <Rinternals.h> via cran.h, and cpp11 needs to set R_NO_REMAP/STRICT_R_HEADERS before any R header is processed
Contributor
|
I think this broke the wasm build 😢 https://github.com/r-universe/tidyverse/actions/runs/25623880286/job/75215538375? Also note this pr commits a merge conflict to NEWS.md 🙈 |
Member
Author
|
Eew, I even used the web interface to resolve the conflict, but obviously did something stupid. I just fixed up NEWS on Are you game to open a proper issue for the WASM thing? Pleasing all the platforms re: header includes feels like whack-a-mole 😬 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #775 and, hopefully, heads off future Alpine issues of this nature.
Exactly why I was ordering includes as I was has been lost in the sands of time. But this seems more conventional:
system → cpp11 → other vendored (libxls, rapidxml) → project
Within the group, alphabetical. Blank lines between.
For a .cpp file, the corresponding .h sits at the top.
Why system before libxls? we want standard headers like
<stdint.h>processed in the global namespace, not inside xls. This is #775. (And pretty similar stories in #562, #676, #714.)Why cpp11 before libxls? libxls transitively pulls in <Rinternals.h> via cran.h, and cpp11 needs to set R_NO_REMAP/STRICT_R_HEADERS before any R header is processed