Repository navigation
diagnose comparisons between two arrays (P2865), fix for [GH #370] - #387
mohitmishra786 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Diagnostic check and operator integration src/error_msg.txt, src/expr.c |
The built-in comparison paths check operands before transformations. They report remarks, warnings, or errors according to the C++ standard and emulated compiler versions. |
Standard and compiler-version coverage tests/tests/changes/GH370*.sft.cpp, tests/tests/changes/.GH370*.rto/*, src/pending_changes/gh-370-array-comparisons.txt |
Tests cover array-to-array and array-to-pointer comparisons across language modes and compiler-version configurations. The pending-change note describes the diagnostics. |
Imported Clang diagnostic expectations tests/tests/imported/clang/cpp/SemaCXX/* |
Expected outputs now include array-comparison remarks, warnings, and errors for the covered cases. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant BuiltinComparisonPath
participant ArrayComparisonCheck
participant CompilerDiagnostics
BuiltinComparisonPath->>ArrayComparisonCheck: Check original operands
ArrayComparisonCheck->>CompilerDiagnostics: Report remark, warning, or error
Merge Risk: 
abd74
-
No actionable merge-blocking risk was identified in these expected-output updates.
Architecture risk: 
abd74
-
The change affects 2 systems.
Changed systems:
tests,srcArchitecture concerns
No architecture-level concerns identified.Review details
-
Systems and components
- observed — tests (service) was modified; 42 changed files map to changed impact.
- observed — src (service) was modified; 3 changed files map to changed impact.
Before / after behavior
- observed — Modified behavior in src/error_msg.txt: Added diagnostics for deprecated array-to-array comparisons, disallowed array comparisons, and comparisons that compare array addresses.
- observed — Modified behavior in src/expr.c: Added helpers that recognize array operands, including array-to-pointer conversions through compiler-generated pointer casts, and diagnose comparisons when both operands qualify. The diagnostic depends on C++ mode and standard version: pre-C++20 uses a remark or Clang 20+ warning; C++20 uses a deprecation warning; C++26 uses an error for GCC 15+ and Clang 20+, otherwise a deprecation warning.
- observed — Modified behavior in src/expr.c: The non-operator-function path in
process_rel_operatornow checks for array comparison before operand transformations. - observed — Modified behavior in src/expr.c: A second non-operator-function comparison path now checks for array comparison before operand transformations.
-
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: diagnosing comparisons between two arrays under P2865. It also references the related issue without adding unrelated detail. Linked Issues check Issue #370requires P2865R6 handling for comparisons between two arrays.src/expr.cchecks built-in equality and relational operators before array-to-pointer conversion. The implementation diagnoses…Out of Scope Changes check The source change, diagnostic messages, pending-change note, and test expectation updates all support issue #370. The imported Clang expectation updates record diagnostics for the same array-compariso…Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Linked Issues check
-
Explanation
Issue
#370requires P2865R6 handling for comparisons between two arrays.src/expr.cchecks built-in equality and relational operators before array-to-pointer conversion. The implementation diagnoses array-to-array comparisons, including class operands converted to arrays. It emits an error in C++26 for the supported compiler-version emulations and a deprecation warning otherwise. It emits the required deprecation warning in C++20 and C++23. Tests cover the operators, language modes, compiler-version emulations, conversion functions, qualification conversion, and unaffected array-to-pointer cases.
Full details: Out of Scope Changes check
-
Explanation
The source change, diagnostic messages, pending-change note, and test expectation updates all support issue
#370. The imported Clang expectation updates record diagnostics for the same array-comparison behavior. The pre-C++20 remark and emulation-specific warning are part of the same diagnostic change. No unrelated change is shown in the reviewable summary.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files. (5 skipped: 5 unsupported.)
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
|
AcknowlEDG review results updated for
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
AcknowlEDG review results updated for
CI success. |
nv-cmeerw
left a comment
nv-cmeerw
left a comment
There was a problem hiding this comment.
This should also handle user-defined conversion functions, e.g.
struct C
{
using arr_t = int[2];
operator arr_t &() const;
};
void g(int (&arr)[2], C c)
{
arr == arr;
c == c;
}
0d4eabd to
ee685d1
Compare
|
@nv-cmeerw fixed in ee685d1 (rebased onto main): class operands converted to arrays by a conversion function ( |
|
AcknowlEDG review results updated for
|
|
AcknowlEDG review results updated for
CI success. |
|
Clang diagnosed this with a warning even before C++20 deprecated it. E.g.: https://godbolt.org/z/KvThjdvqv Should we perhaps do something similar with this change? |
daveedvdv-nvidia
left a comment
daveedvdv-nvidia
left a comment
There was a problem hiding this comment.
Looks good.
I have a question in the conversation about possibly extending the diagnosis to pre-C++20 modes.
|
@daveedvdv-nvidia @nv-cmeerw Thanks, good point. I checked this on Godbolt (x86-64), with no extra warning options:
So my suggestion: before C++20, issue a remark, the way a comma operator in a subscript is handled before C++20, and a warning when emulating Clang 20 or later. It would get its own message, e.g. "comparison between two arrays compares their addresses", since it is not deprecated yet in those modes. C++20 and later stay as in this PR. Does that sound OK, or would you prefer a warning in all modes before C++20? |
|
Yes, the remark idea sounds exactly right — thanks! |
|
There still appears to be an issue with cv-qualifications: |
ee685d1 to
197630c
Compare
|
@nv-cmeerw @daveedvdv-nvidia updated in 197630c:
|
|
AcknowlEDG review results updated for
|
|
AcknowlEDG review results updated for
CI failure. |
|
AcknowlEDG review results updated for
CI success. |
The requested changes have been made.
There was a problem hiding this comment.
This looks great!
But I have some suggested changes nonetheless...
See my comment suggesting making the C++26-mode error discretionary.
Also, I have suggested added tests (attached as diffs for the existing test files).
Ah, and one more thing: Can you rebase and squash into a single commit?
| !clangcpp_version_is(<200000)) { | ||
| /* GCC 15 and Clang 20 implement P2865. Earlier versions only issue a | ||
| deprecation warning. */ | ||
| expr_pos_error(ec_array_comparison_not_allowed, operator_pos); |
There was a problem hiding this comment.
Maybe use expr_pos_diagnostic(es_discretionary_error, ...) here.
That way the error severity can be adjusted by the programmer if needed.
(I think Clang and GCC allow that as well.)
There was a problem hiding this comment.
Done in 7083ca5: the C++26 error now uses expr_pos_diagnostic(es_discretionary_error, ...).
A comparison between two arrays with an equality or relational operator is now a discretionary error in C++26, a deprecation warning in C++20 and C++23, and a remark before C++20 (a warning when emulating Clang 20 or later). When emulating GCC before 15 or Clang before 20, C++26 gets only the warning. Class operands converted to arrays by a conversion function (including through an implicit qualification conversion) are covered too. Co-authored-by: Daveed Vandevoorde <dvandevoorde@nvidia.com>
abd74f2 to
7083ca5
Compare
|
@daveedvdv-nvidia thanks! Rebased onto main and squashed into one commit (7083ca5), with you as co-author for the tests:
Could you please take another look? |
|
AcknowlEDG review results updated for
CI success. |
|
AcknowlEDG review results updated for
|
Fixes #370.
A comparison between two arrays with
==,!=,<,<=,>or>=was accepted silently in every mode. It is deprecated in C++20 and not allowed in C++26 (P2865).process_eq_operatorandprocess_rel_operatornow callcheck_for_array_comparisonbefore the array-to-pointer conversion (the same place<=>already rejects two arrays). If both operands have array type:-std=c++26(checked with the gcc:14 and gcc:15 images); Clang implemented P2865 in Clang 20.A comparison between an array and a pointer is not affected.
Summary by CodeRabbit
Bug Fixes