Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 22, 2025

This PR addresses clang-tidy warnings about reference members in the InsertingVisitor struct. The warnings were flagged by the cppcoreguidelines-avoid-const-or-ref-data-members check.

Changes Made

Added a targeted suppression using NOLINTBEGIN/NOLINTEND blocks to both reference members with an explanatory comment:

struct InsertingVisitor {
  // NOLINTBEGIN(cppcoreguidelines-avoid-const-or-ref-data-members): Lifetime of referenced objects is guaranteed beyond visitor lifetime in this pattern
  JEvent& m_event;
  const std::string& m_collection_name;
  // NOLINTEND(cppcoreguidelines-avoid-const-or-ref-data-members)
  // ...
};

Justification

The NOLINTBEGIN/NOLINTEND block approach is preferred over inline NOLINT comments because it provides format-stable suppression that isn't dependent on line positioning, preventing issues when code formatters reflow long comment lines.

Both reference members are safe in this specific usage pattern because:

  1. The InsertingVisitor is created and used immediately within the same scope in GetEvent()
  2. The referenced objects (event and coll_name) come from the calling context and have guaranteed lifetimes that extend beyond the visitor's usage
  3. The visitor is destroyed at the end of the loop iteration, well before the referenced objects go out of scope

This follows the established pattern in the codebase for suppressing specific clang-tidy warnings with appropriate justification comments.

Fixes #1981.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Add NOLINT(cppcoreguidelines-avoid-const-or-ref-data-members) suppression to the
m_collection_name member in InsertingVisitor struct. The lifetime of the referenced
string is guaranteed to outlive the visitor in this usage pattern, as the visitor
is created and used immediately within the same scope where the collection name
string exists.

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] clang-tidy warning regarding m_collection_name being a reference Add clang-tidy suppression for InsertingVisitor reference member Aug 22, 2025
@Copilot Copilot AI requested a review from wdconinc August 22, 2025 13:47
Copilot finished work on behalf of wdconinc August 22, 2025 13:47
@wdconinc wdconinc added the pre-commit.ci autofix Enable pre-commit.ci autofixes even for bot accounts label Aug 22, 2025
@pre-commit-ci pre-commit-ci bot removed the pre-commit.ci autofix Enable pre-commit.ci autofixes even for bot accounts label Aug 22, 2025
…ability

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
@Copilot Copilot AI changed the title Add clang-tidy suppression for InsertingVisitor reference member Add clang-tidy suppression for InsertingVisitor reference member using NOLINTBEGIN/NOLINTEND blocks Aug 22, 2025
@Copilot Copilot AI requested a review from wdconinc August 22, 2025 13:53
Copilot finished work on behalf of wdconinc August 22, 2025 13:53
@wdconinc wdconinc requested a review from veprbl August 22, 2025 13:57
@wdconinc wdconinc marked this pull request as ready for review August 22, 2025 13:57
@wdconinc wdconinc enabled auto-merge August 22, 2025 13:57
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

@Copilot Copilot AI requested a review from wdconinc August 22, 2025 14:33
Copilot finished work on behalf of wdconinc August 22, 2025 14:34
@wdconinc
Copy link
Contributor

@copilot Please include the JEvent reference in the NOLINT block :-)

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
auto-merge was automatically disabled August 22, 2025 14:47

Head branch was pushed to by a user without write access

Copy link
Contributor Author

Copilot AI commented Aug 22, 2025

@copilot Please include the JEvent reference in the NOLINT block :-)

Included the JEvent reference in the NOLINT block. Both reference members are now covered by the same suppression block. (commit: 8a0857e)

@Copilot Copilot AI changed the title Add clang-tidy suppression for InsertingVisitor reference member using NOLINTBEGIN/NOLINTEND blocks Add clang-tidy suppression for InsertingVisitor reference members using NOLINTBEGIN/NOLINTEND blocks Aug 22, 2025
Copilot finished work on behalf of wdconinc August 22, 2025 14:48
@github-actions github-actions bot dismissed their stale review August 22, 2025 15:51

No Clang-Tidy warnings found so I assume my comments were addressed

@wdconinc wdconinc enabled auto-merge August 22, 2025 17:32
@Copilot Copilot AI temporarily deployed to github-pages August 22, 2025 18:54 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-tidy warning regarding m_collection_name being a reference
2 participants