Skip to content

Follow cppstyle include order in ops#5996

Open
wujingyue wants to merge 3 commits intomainfrom
wjy/include
Open

Follow cppstyle include order in ops#5996
wujingyue wants to merge 3 commits intomainfrom
wjy/include

Conversation

@wujingyue
Copy link
Collaborator

Summary

  • switch ops headers to quoted includes
  • reorder includes to match Google cppstyle in csrc/ops

Testing

  • lintrunner

@github-actions
Copy link

github-actions bot commented Feb 21, 2026

Review updated until commit f1dddf4

Description

  • Add valueOrError utility function in base.h for safe optional value extraction

  • Replace .value() calls with valueOrError throughout codebase for improved safety

  • Switch ops module headers from angle brackets to quoted includes

  • Reorder includes to follow Google cppstyle guidelines in csrc/ops

Changes walkthrough

Relevant files
Enhancement
21 files
base.h
Add valueOrError template function for safe optional extraction
+20/-1   
alias.cpp
Switch to quoted includes and use valueOrError utility     
+12/-11 
arith.cpp
Switch to quoted includes and use valueOrError utility     
+10/-16 
composite.cpp
Switch to quoted includes and reorder for cppstyle             
+9/-7     
indexing.cpp
Switch to quoted includes and reorder for cppstyle             
+9/-8     
normalization.cpp
Switch to quoted includes and reorder for cppstyle             
+6/-5     
schedule.cpp
Switch to quoted includes and reorder for cppstyle             
+3/-2     
utils.cpp
Switch to quoted includes and use valueOrError utility     
+13/-11 
alias.h
Switch to quoted includes and reorder for cppstyle             
+6/-7     
all_ops.h
Switch to quoted includes and reorder for cppstyle             
+7/-6     
arith.h
Switch to quoted includes and reorder for cppstyle             
+8/-9     
composite.h
Switch to quoted includes and reorder for cppstyle             
+5/-6     
indexing.h
Switch to quoted includes and reorder for cppstyle             
+4/-5     
normalization.h
Switch to quoted includes and reorder for cppstyle             
+5/-6     
schedule.h
Switch to quoted includes and reorder for cppstyle             
+5/-6     
utils.h
Switch to quoted includes and reorder for cppstyle             
+10/-9   
allocation.cpp
Use valueOrError and std::ranges algorithms                           
+14/-22 
grid_serialization.cpp
Use valueOrError for optional extraction                                 
+4/-4     
evaluator.cpp
Use valueOrError for optional extraction                                 
+2/-2     
jit.cpp
Use valueOrError for optional extraction                                 
+2/-5     
lower_to_communication.cpp
Use valueOrError for optional extraction                                 
+3/-7     
Additional files
8 files
internal_nodes.cpp +2/-3     
parallel_dimension_map.cpp +1/-2     
executor.cpp +1/-2     
fusion_kernel_runtime.cpp +2/-4     
matmul_utils.cpp +2/-4     
utils.cpp +1/-2     
vectorize_helper.cpp +1/-2     
python_translate.cpp +3/-4     

PR Reviewer Guide

Here are some key observations to aid the review process:

🧪 No relevant tests
⚡ Recommended focus areas for review
New utility function

The new valueOrError template function is introduced to safely extract values from std::optional. This is a good practice for clang-tidy compliance, but reviewers should verify that all call sites where .value() was replaced actually had proper error handling and that the new function provides the same guarantees.

//! Returns the value of an optional, or throws via NVF_ERROR if nullopt.  This
//! is to satisfy clang-tidy bugprone-unchecked-optional-access.  Use this when
//! you have already ensured that the optional is engaged.
template <typename T>
const T& valueOrError(const std::optional<T>& opt) {
  NVF_ERROR(opt.has_value());
  return *opt;
}
template <typename T>
T& valueOrError(std::optional<T>& opt) {
  NVF_ERROR(opt.has_value());
  return *opt;
}
template <typename T>
T valueOrError(std::optional<T>&& opt) {
  NVF_ERROR(opt.has_value());
  return std::move(opt).value();
}
Removed template function

The local valueOrError template function that was previously defined in arith.cpp has been removed since it's now available in base.h. Reviewers should ensure this doesn't break any compilation or that there are no other references to this local version.

Val* castOp(DataType dtype, Val* v1) {
  auto orig_dtype = valueOrError(v1->getDataType());
  if (dtype == orig_dtype) {

Include style changes
Headers are being converted from angle-bracket includes (#include <...>) to quoted includes (#include "...") and reordered according to Google C++ style guide. While this is generally good practice, reviewers should verify that the quoted includes are appropriate for internal headers and that the new ordering doesn't cause any dependency issues.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 21, 2026

Greptile Summary

This PR refactors include statements across the csrc/ops/ directory to follow Google C++ Style Guide conventions. The changes include converting angle-bracket includes to quoted includes for local headers and reordering them alphabetically. Beyond formatting, the PR incorporates modern C++ improvements surfaced by clang-tidy:

  • Modernized standard library usage with std::ranges algorithms (std::ranges::all_of, std::ranges::find, std::ranges::transform)
  • Safer optional value access using valueOrError() helper and std::move(opt).value()
  • Added explicit enum base types and default member initializers
  • Adopted C++20 designated initializers for struct construction

All changes are backwards-compatible refactorings that improve code safety and maintainability without altering functionality. The PR was validated with lintrunner.

Confidence Score: 5/5

  • This PR is safe to merge - pure refactoring with modern C++ improvements
  • All changes are non-functional refactorings that follow established style guidelines and modern C++ best practices. The include reordering is systematic and consistent, while code modernizations (std::ranges, designated initializers, safer optional access) improve code quality without changing behavior. Testing with lintrunner confirms correctness.
  • No files require special attention

Important Files Changed

Filename Overview
csrc/base.h Improved optional value access using std::move(opt).value() instead of dereferencing
csrc/device_lower/pass/allocation.cpp Modernized to use std::ranges algorithms and designated initializers for safer struct construction
csrc/ops/utils.cpp Added required includes, modernized with std::ranges::transform and safer valueOrError() calls
csrc/ops/utils.h Added explicit enum base type and default member initializer for safer code

Last reviewed commit: 023624b

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

17 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

29 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

29 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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.

2 participants