-
Notifications
You must be signed in to change notification settings - Fork 63
Add support for specifiying a language standard to bmake #234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for specifiying a language standard to bmake #234
Conversation
* docs/templates/bmake.txt:
* templates/bmake.mpd:
WalkthroughAdds a new templated variable languagestandard to the bmake documentation and hooks it into the build template so CCFLAGS can include -std= when defined. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/templates/bmake.txt (1)
22-22: Clarify scope (C++-only) and compiler support in the description of languagestandardMake it explicit this affects C++ compilation and only when the compiler understands -std=… to avoid confusion across compilers.
Proposed tweak:
-languagestandard = Which version of ISO C++ standard to compile against (c++14, c++17, c++20, c++23) +languagestandard = C++ language standard to compile against (e.g., c++14, c++17, c++20, c++23). Applied only when the selected compiler supports -std=...Also, please confirm whether this template is ever used with compilers that do not support -std= (e.g., non-GNU/Clang style), so we can consider adding a compiler-conditional guard if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/templates/bmake.txt(1 hunks)templates/bmake.mpd(1 hunks)
⏰ 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). (6)
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022Release32
- GitHub Check: windows-2022 vs2022
- GitHub Check: macos-14-C++17
- GitHub Check: macos-13-C++17
- GitHub Check: ubuntu-24.04 g++-10
| RC = <%rc%> | ||
| LIBFLAGS = <%libflags%> | ||
| CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%> | ||
| CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%><%if(languagestandard)%> -std=<%languagestandard%><%endif%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Do not append -std=… to CCFLAGS; it will be applied to C sources too and can break C builds
CCFLAGS is used for both C and C++ compilation rules in this template (.c, .cc, .cxx, .cpp, .C). Adding a C++ -std flag here will propagate to .c compilations, which Clang/GCC will reject (e.g., “invalid argument '-std=c++17' not allowed with 'C'”). This is a latent build break for projects containing C sources.
Minimal fix within this line:
-CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%><%if(languagestandard)%> -std=<%languagestandard%><%endif%>
+CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%>Follow-up (outside this hunk): introduce a separate flag used only by C++ rules and inject it into C++-only compile patterns (.cpp/.cxx/.cc/.C), not .c. Example:
Make variable definition near CCFLAGS:
CXX_STD_FLAG =<%if(languagestandard)%> -std=<%languagestandard%><%endif%>
Update compile commands for C++ sources by adding
- Parallel .cpp rule (Line 333):
From:
$(CC) ... $ (CCFLAGS) $(CFLAGS) ...
To:
$(CC) ... $ (CCFLAGS)$(CXX_STD_FLAG) $ (CFLAGS) ... - .cpp/.cxx/.cc/.C rules (Lines 338, 343, 347, 351):
From:
$(CC) ... $ (CCFLAGS) $(CFLAGS) ...
To:
$(CC) ... $ (CCFLAGS)$(CXX_STD_FLAG) $ (CFLAGS) ... - Per-file rule with transdir/buildflags (Line 361):
From:
$(CC) ... $ (CCFLAGS) $(CFLAGS) ...
To:
$(CC) ... $ (CCFLAGS)$(CXX_STD_FLAG) $ (CFLAGS) ...
Please confirm whether projects using this template include any .c sources. If they never do, we can keep the current approach; otherwise, the refactor above avoids C builds failing when languagestandard is set.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show templates/bmake.mpd (first 220 lines) with numbers ==="
if [ -f templates/bmake.mpd ]; then
nl -ba templates/bmake.mpd | sed -n '1,220p'
else
echo "templates/bmake.mpd not found"
fi
echo
echo "=== Search for 'languagestandard' in repository ==="
rg "languagestandard" -n || true
echo
echo "=== Search for CCFLAGS definitions/usages across repo ==="
rg "CCFLAGS" -n || true
echo
echo "=== Show parts of templates/bmake.mpd around common compile rules (lines 300-420) ==="
if [ -f templates/bmake.mpd ]; then
nl -ba templates/bmake.mpd | sed -n '300,420p'
fiLength of output: 327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="templates/bmake.mpd"
echo "=== Show templates/bmake.mpd lines 1-220 ==="
if [ -f "$FILE" ]; then
awk 'NR>=1 && NR<=220{printf("%4d: %s\n", NR, $0)}' "$FILE"
else
echo "$FILE not found"
fi
echo
echo "=== Show templates/bmake.mpd lines 280-420 ==="
if [ -f "$FILE" ]; then
awk 'NR>=280 && NR<=420{printf("%4d: %s\n", NR, $0)}' "$FILE"
fi
echo
echo "=== Search for 'languagestandard' in repo ==="
if command -v rg >/dev/null 2>&1; then
rg -n "languagestandard" || true
else
grep -RIn "languagestandard" || true
fi
echo
echo "=== Search for CCFLAGS, CFLAGS, CXXFLAGS, and CXX occurrences in repo ==="
if command -v rg >/dev/null 2>&1; then
rg -n "CCFLAGS|CFLAGS|CXXFLAGS|CXX" || true
else
grep -RIn "CCFLAGS\|CFLAGS\|CXXFLAGS\|CXX" || true
fi
echo
echo "=== Show any lines mentioning -std= in the repo ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore-vcs "\-std=" || true
else
grep -RIn --binary-files=without-match "\-std=" || true
fiLength of output: 23905
Do not put -std= into CCFLAGS — it will be passed to C compilations and can break C builds
Short reason: templates/bmake.mpd currently places -std=<%languagestandard%> inside CCFLAGS (confirmed at templates/bmake.mpd:100). CCFLAGS is used by the .c rule and per-file rules (see templates/bmake.mpd:353–355, 357–362), so C compiler invocations will receive C++ -std flags and may fail.
Files/locations to change
- templates/bmake.mpd
- Line 100: remove the <%if(languagestandard)%> -std=... piece from CCFLAGS.
- Insert a C++-only variable right after CCFLAGS, e.g. CXX_STD_FLAG.
- Update all C++ compile rules to use
$(CXX_STD_FLAG): parallel .cpp rule (≈line 333), .cpp/.cxx/.cc/.C rules (≈338, 343, 347, 351), and the per-file rule block (≈357–362). For the per-file rules, add $ (CXX_STD_FLAG) only when the source file is C++ (use the template conditional that checks the source-file language or extension).
Minimal diffs (apply to templates/bmake.mpd)
-CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%><%if(languagestandard)%> -std=<%languagestandard%><%endif%>
+CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%>
+# C++-only standard flag
+CXX_STD_FLAG =<%if(languagestandard)%> -std=<%languagestandard%><%endif%>Example compile-rule change (repeat for .cpp/.cxx/.cc/.C and parallel .cpp rules):
- $(CC) ... $(CCFLAGS) $(CFLAGS) $(WARN_FLAGS) -c ...
+ $(CC) ... $(CCFLAGS) $(CXX_STD_FLAG) $(CFLAGS) $(WARN_FLAGS) -c ...For per-file rules generated for each source file, conditionally add $(CXX_STD_FLAG) only when that source file is C++:
... $(CCFLAGS) <%if(source-file-is-cxx)%> $(CXX_STD_FLAG)<%endif%> $(CFLAGS) ...
Please confirm whether projects using this template contain any .c sources; if they do, apply the fix above. If none contain C sources, leaving the current template is harmless but fragile if a .c is later added.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%><%if(languagestandard)%> -std=<%languagestandard%><%endif%> | |
| CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%> | |
| # C++-only standard flag | |
| CXX_STD_FLAG =<%if(languagestandard)%> -std=<%languagestandard%><%endif%> |
🤖 Prompt for AI Agents
In templates/bmake.mpd around line 100, remove the <%if(languagestandard)%>
-std=<%languagestandard%><%endif%> piece from CCFLAGS so -std= is not passed to
C compilations; immediately after CCFLAGS add a new variable (e.g. CXX_STD_FLAG)
that expands to <%if(languagestandard)%> -std=<%languagestandard%><%endif%>;
update all C++ compile rules to use $(CXX_STD_FLAG) instead of relying on
CCFLAGS — specifically the parallel .cpp rule (~line 333) and the
.cpp/.cxx/.cc/.C rules (~lines 338, 343, 347, 351) — and in the per-file rule
block (~lines 357–362) conditionally append $(CXX_STD_FLAG) only when the source
file is C++ using the template conditional for source-file language/extension;
finally, verify whether projects using this template contain any .c sources and
if so apply these changes, otherwise note that leaving -std in CCFLAGS would be
fragile if a .c file is later added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
templates/bmake.mpd (1)
100-100: Confirm intended precedence for -std: it currently overrides -std set via ccflags/compile_flagsPlacing languagestandard last means it wins when multiple -std flags are present. If you prefer user-specified compile_flags to take precedence, swap the order.
Apply this diff to let compile_flags override languagestandard:
-CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%><%if(languagestandard)%> -std=<%languagestandard%><%endif%> +CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(languagestandard)%> -std=<%languagestandard%><%endif%><%if(compile_flags)%> <%compile_flags%><%endif%>docs/templates/bmake.txt (1)
22-22: Tighten wording and punctuation; clarify C++-only intentMinor grammar fix and explicit note that it pertains to C++ sources. Also adds terminal punctuation for consistency.
-languagestandard = Which version of ISO C++ standard to compile against (c++14, c++17, c++20, c++23) +languagestandard = Specify the ISO C++ standard for C++ sources (e.g., c++14, c++17, c++20, c++23).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/templates/bmake.txt(1 hunks)templates/bmake.mpd(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/templates/bmake.txt
[grammar] ~22-~22: There might be a mistake here.
Context: ...table. languagestandard = Which version of ISO C++ standard to compile against (c+...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...ile against (c++14, c++17, c++20, c++23) libflags = Flags that are required to cr...
(QB_NEW_EN)
⏰ 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). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022Release32
- GitHub Check: macos-14-C++17
- GitHub Check: macos-13-C++17
- GitHub Check: ubuntu-24.04 g++-10
| CCFLAGS = $(CC_CFLAGS)<%if(type_is_binary)%> $(BINARY_FLAGS)<%endif%><%if(compile_flags)%> <%compile_flags%><%endif%><%if(languagestandard)%> -std=<%languagestandard%><%endif%> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
C++ standard flag may be applied to .c compilations — verify this is acceptable or scope to C++ rules only
CCFLAGS is used by all compile rules, including .c. Appending -std=c++NN here may (a) be rejected by some compilers for C files, or (b) flip .c files into C++ mode. This can silently change semantics or fail builds in mixed C/C++ projects.
- If all supported Borland/Embarcadero compilers used via this template accept -std=c++NN on .c inputs (or always compile as C++), then OK.
- Otherwise, consider moving the -std=<%languagestandard%> addition into only the C++ pattern rules (.cpp/.cxx/.cc/.C) instead of CCFLAGS.
I can provide a follow-up patch that scopes the flag to C++-only rules if you confirm this is needed.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository root: $(pwd)"
echo
echo "1) Show templates/bmake.mpd (first 400 lines):"
echo "------------------------------------------------"
if [ -f "templates/bmake.mpd" ]; then
sed -n '1,400p' templates/bmake.mpd || true
else
echo "templates/bmake.mpd not found"
fi
echo
echo "2) Search for CCFLAGS, -std and compile rule patterns:"
echo "------------------------------------------------"
rg -n --hidden -S 'CCFLAGS|\-std=|languagestandard|%.o: %.c|%.o: %.cpp|%.o: %.cxx|%.o: %.cc|\.c:|\.cpp:|\.cxx:|\.cc:' || true
echo
echo "3) Show lines around occurrences of CCFLAGS:"
echo "------------------------------------------------"
rg -n --hidden -S -n 'CCFLAGS' -n --line-number -n -H -S || true
echo
echo "4) Grep for common compile command usages (CC, CXX, CFLAGS, CXXFLAGS):"
echo "------------------------------------------------"
rg -n --hidden -S '\$\(CC\)|\$\(CXX\)|CFLAGS|CXXFLAGS' || trueLength of output: 27004
Scope -std=<%languagestandard%> to C++ compile rules in templates/bmake.mpd
Verified: CCFLAGS contains -std=<%languagestandard%> and that CCFLAGS is used by all compile rules (including .c), so a C++ standard flag will be passed to C compilations.
Files/locations to fix
- templates/bmake.mpd:100 — CCFLAGS includes
<%if(languagestandard)%> -std=<%languagestandard%><%endif%> - templates/bmake.mpd:333–361 — .cpp/.cxx/.cc/.C/.c compile rules all use
$(CCFLAGS)(so the -std flag will reach .c compilations)
Suggested change (minimal)
- Remove the languagestandard bit from CCFLAGS and add it only to C++ rules (or introduce a new CPP-specific var):
Original:
CCFLAGS = $(CC_CFLAGS)...<%if(languagestandard)%> -std=<%languagestandard%><%endif%>
Change to:
CCFLAGS = $(CC_CFLAGS)... # no -std here
And in the C++ rules (example non-parallel .cpp rule) append the flag:
.cpp$(OBJ_EXT):
$(CC) ... $(CCFLAGS) $(CFLAGS) <%if(languagestandard)%> -std=<%languagestandard%><%endif%> $(WARN_FLAGS) -c -o $(@D)\$(@F) $<
If you want, I can prepare a PR that moves the -std into the C++ rules (or introduces a CPP_STD_FLAGS variable and uses it only for the C++ patterns).
🤖 Prompt for AI Agents
In templates/bmake.mpd around lines 100 and 333–361, CCFLAGS currently contains
the C++ standard flag which gets applied to C compilations; remove the
<%if(languagestandard)%> -std=<%languagestandard%><%endif%> fragment from
CCFLAGS and instead add the standard flag only to C++ compile rules by either
(A) introducing a new CPP_STD_FLAGS (or CPPFLAGS/CPP_CFLAGS) conditional var
that expands to <%if(languagestandard)%> -std=<%languagestandard%><%endif%> and
appending that var to all .cpp/.cxx/.cc/.C pattern rules, or (B) directly
appending the existing <%if(languagestandard)%>
-std=<%languagestandard%><%endif%> to each C++ pattern recipe; ensure C pattern
rules keep using only CCFLAGS so they do not receive the C++ -std flag.
Summary by CodeRabbit
New Features
Documentation