-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[None][fix] Fix const modifier inconsistency in log function declaration/implementation #6679
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
Conversation
…rsion Signed-off-by: fanyunfan <2569548856@qq.com>
…tation Signed-off-by: fanyunfan <2569548856@qq.com>
📝 WalkthroughWalkthroughThe update adds Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 comments)
Other keywords and placeholders
Documentation and Community
|
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)
cpp/include/tensorrt_llm/common/logger.h (1)
55-60: Unnecessaryconston pass-by-value parametersAdding
constto parameters that are already passed by value (Level const level,int const rank) does not provide extra safety—the caller still passes a copy that the callee cannot mutate externally. Most C++ style guides discourage this because it clutters the signature without benefit and may mislead users into thinking a reference is involved.If the intent is to highlight immutability, prefer documenting it in the comment rather than on primitive-type value parameters.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/include/tensorrt_llm/common/logger.h(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.{cpp,h,hpp,cc,cxx}: Closing braces of namespaces should have a comment saying the namespace it closes (e.g., } // namespace foo).
Prefer const or constexpr variables over #defines whenever possible.
A variable that is not modified after its initialization should be declared as const.
Except 0 (used for checking signness/existence/emptiness), nullptr, true, false, all other literals should only be used for variable initialization.
Use the Allman indentation style for braces in C++ code.
Put the semicolon for an empty for or while loop in a new line.
The statement forming the body of a switch, while, do..while, or for statement shall be a compound statement (use brace-delimited statements).
If and else should always be followed by brace-delimited statements, even if empty or a single statement.
C++ filenames should use camel case with the first letter lowercase (e.g., thisIsAFilename.cpp), and all files involved in a compilation target must have case-insensitive unique filenames.
All types (including class names) should use camel case with uppercase first letter (e.g., FooBarClass).
Local variables, methods, and namespaces should use camel case with first letter lowercase (e.g., localFooBar).
Non-magic-number global variables that are non-static and not defined in anonymous namespace should use camel case prefixed by 'g' (e.g., gDontUseGlobalFoos).
Non-magic-number global variables that are static or defined in an anonymous namespace should use camel case prefixed by 's' (e.g., sMutableStaticGlobal).
Locally visible static variables should use camel case with lowercase prefix 's' as the first letter (e.g., static std::once_flag sFlag;).
Class member variables should use camel case prefixed with 'm' (e.g., mNbFooValues). Public member variables do not require the 'm' prefix but it is encouraged for clarity.
Enumerations, global constants, static constants at class-scope, and function-scope magic-number/literal constants should be uppercase snake case with prefix...
Files:
cpp/include/tensorrt_llm/common/logger.h
**/*.{h,hpp}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Use a preprocessor guard in header files. The guard name must have prefix TRTLLM_ followed by the filename, all in caps, and no trailing underscore.
Files:
cpp/include/tensorrt_llm/common/logger.h
**/*.{cpp,h,hpp,cc,cxx,cu,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
Files:
cpp/include/tensorrt_llm/common/logger.h
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : When specifying pointers to const data, the pointer itself may be const in some use cases (e.g., char const * const errStr = getErrorStr(status);).
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : A variable that is not modified after its initialization should be declared as const.
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : Prefer const or constexpr variables over #defines whenever possible.
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : Parameter names should be consistent across function definition and corresponding function declarations.
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : Casting should not remove any const or volatile qualification from the type of a pointer or reference.
📚 Learning: applies to **/*.{cpp,h,hpp,cc,cxx} : a variable that is not modified after its initialization should...
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : A variable that is not modified after its initialization should be declared as const.
Applied to files:
cpp/include/tensorrt_llm/common/logger.h
📚 Learning: applies to **/*.{cpp,h,hpp,cc,cxx} : prefer const or constexpr variables over #defines whenever poss...
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : Prefer const or constexpr variables over #defines whenever possible.
Applied to files:
cpp/include/tensorrt_llm/common/logger.h
📚 Learning: applies to **/*.{cpp,h,hpp,cc,cxx} : casting should not remove any const or volatile qualification f...
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : Casting should not remove any const or volatile qualification from the type of a pointer or reference.
Applied to files:
cpp/include/tensorrt_llm/common/logger.h
📚 Learning: applies to **/*.{cpp,h,hpp,cc,cxx} : when specifying pointers to const data, the pointer itself may ...
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : When specifying pointers to const data, the pointer itself may be const in some use cases (e.g., char const * const errStr = getErrorStr(status);).
Applied to files:
cpp/include/tensorrt_llm/common/logger.h
⏰ 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). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (2)
cpp/include/tensorrt_llm/common/logger.h (2)
71-80: Const-correct overloads forward correctly – LGTMThe
std::stringoverloads simply forward to thechar const*versions and now use the sameconstqualifiers, eliminating the earlier inconsistency. Behaviour remains unchanged.
138-160: Implementation consistent with declaration but still inherits attribute problemThe implementation matches the new signature, so no functional issue here. Once the attribute indexes above are adjusted, compile-time format checking will also apply correctly.
|
Dear @kaiyux ,I noticed that you have made some related commit about this part(cpp/include/tensorrt_llm/common/logger.h), so I was wondering if you would like to take a look at this PR I submitted when you have some free time?
|
|
It seems that another reviewer has no time these days. @tongyuantongyu , thank you so much for your guidance on my previous two PRs—I really appreciate it! Would you mind taking a look at this one when you have time? Sorry to trouble you again; I’m still not very familiar with other community members.
|
|
/bot run |
|
PR_Github #15688 [ run ] triggered by Bot |
|
PR_Github #15688 [ run ] completed with state |
|
/bot run |
|
PR_Github #15771 [ run ] triggered by Bot |
|
PR_Github #15771 [ run ] completed with state |
|
/bot run |
|
PR_Github #15843 [ run ] triggered by Bot |
|
PR_Github #15843 [ run ] completed with state |
…ion/implementation (NVIDIA#6679) Signed-off-by: fanyunfan <2569548856@qq.com> Co-authored-by: fanyunfan <2569658856@qq.com> Co-authored-by: Yunfan Fan <46273019+fyf2016@users.noreply.github.com> Signed-off-by: Yuxin <yuxinz@nvidia.com>
|
@tongyuantongyu Greatness needs no explanation! Thank you for your review. I've become your follower and look forward to learning more from you in the future.
|



Problem
Issue 1: Inconsistency between declaration and implementation of the log template function with rank parameter and char const* format.
While this doesn't cause compilation errors, it may mislead users into thinking these parameters are modifiable (when they're actually treated as read-only in the implementation). From both syntactic correctness and API design consistency perspectives, the declaration and implementation should be aligned.
Declaration:
Implementation:
Issue 2: Inconsistent const Usage Between std::string const& Format Log Functions
For the log template functions with std::string const& format:
- Implementation observations:
Recommended Fix
Since no parameter modification occurs in either function:
Avoids user confusion about parameter mutability
Matches the immutable behavior of internal function calls
Aligns with the principle of least surprise
isEnabled Function:
getPrefix Function:
Current Implementation
Declaration of log template function with char const* format.
Implementation of log template function with char const* format.
Implementation of log template function with std::string const& format.
Solution
Apply const qualifiers uniformly to maintain const consistency in both declaration and implementation for the log template function.
Summary by CodeRabbit