KEMBAR78
[None][fix] Fix const modifier inconsistency in log function declaration/implementation by Fan-Yunfan · Pull Request #6679 · NVIDIA/TensorRT-LLM · GitHub
Skip to content

Conversation

@Fan-Yunfan
Copy link
Contributor

@Fan-Yunfan Fan-Yunfan commented Aug 7, 2025

Problem

Issue 1: Inconsistency between declaration and implementation of the log template function with rank parameter and char const* format.

  • The function declaration does not have const qualifiers for level and rank parameters
  • The implementation incorrectly adds const qualifiers to these parameters

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:

image

Implementation:

image

Issue 2: Inconsistent const Usage Between std::string const& Format Log Functions

For the log template functions with std::string const& format:

  • The version with rank parameter and the version without rank exhibit inconsistent const qualifier usage
  • These functions are essentially wrappers of the functions mentioned in Issue 1, adapted for std::string const& compatibility

- Implementation observations:

  • Neither function modifies level or rank parameters internally
  • All internally called functions (getPrefix(), isEnabled()) use const-qualified parameters

Recommended Fix

Since no parameter modification occurs in either function:

  1. Both overloads should consistently use const qualifiers
  2. Rationale for standardization:
  • Avoids user confusion about parameter mutability

  • Matches the immutable behavior of internal function calls

  • Aligns with the principle of least surprise

image

isEnabled Function:

image

getPrefix Function:

image

Current Implementation

Declaration of log template function with char const* format.

#if defined(_MSC_VER)
    template <typename... Args>
    void log(Level level, char const* format, Args const&... args);

    template <typename... Args>
    void log(Level level, int rank, char const* format, Args const&... args);
#else
    template <typename... Args>
    void log(Level level, char const* format, Args const&... args) __attribute__((format(printf, 3, 0)));

    template <typename... Args>
    void log(Level level, int rank, char const* format, Args const&... args) __attribute__((format(printf, 4, 0)));
#endif

Implementation of log template function with char const* format.

template <typename... Args>
void Logger::log(Logger::Level level, char const* format, Args const&... args)
{
    if (isEnabled(level))
    {
        auto const fmt = getPrefix(level) + format;
        auto& out = level_ < WARNING ? std::cout : std::cerr;
        if constexpr (sizeof...(args) > 0)
        {
            out << fmtstr(fmt.c_str(), args...);
        }
        else
        {
            out << fmt;
        }
        out << std::endl;
    }
}

template <typename... Args>
void Logger::log(Logger::Level const level, int const rank, char const* format, Args const&... args)
{
    if (isEnabled(level))
    {
        auto const fmt = getPrefix(level, rank) + format;
        auto& out = level_ < WARNING ? std::cout : std::cerr;
        if constexpr (sizeof...(args) > 0)
        {
            out << fmtstr(fmt.c_str(), args...);
        }
        else
        {
            out << fmt;
        }
        out << std::endl;
    }
}

Implementation of log template function with std::string const& format.

template <typename... Args>
void log(Level level, std::string const& format, Args const&... args)
{
    return log(level, format.c_str(), args...);
}

template <typename... Args>
void log(Level const level, int const rank, std::string const& format, Args const&... args)
{
    return log(level, rank, format.c_str(), args...);
}

void log(std::exception const& ex, Level level = Level::ERROR);

Level getLevel() const
{
    return level_;
}

Solution

Apply const qualifiers uniformly to maintain const consistency in both declaration and implementation for the log template function.

Summary by CodeRabbit

  • Refactor
    • Improved internal consistency by updating method signatures for enhanced parameter const correctness. No changes to user-facing functionality.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 7, 2025

📝 Walkthrough

Walkthrough

The update adds const qualifiers to the level and rank parameters in all template log method declarations and definitions within the Logger class. This adjustment applies to both Windows-specific and non-Windows-specific code, affecting only function signatures without altering logic or behavior.

Changes

Cohort / File(s) Change Summary
Logger Method Const Qualification
cpp/include/tensorrt_llm/common/logger.h
Added const qualifiers to level and rank parameters in all template log method signatures and definitions for the Logger class. No logic or control flow 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 Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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: Unnecessary const on pass-by-value parameters

Adding const to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 780d750 and 5f4efee.

📒 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 – LGTM

The std::string overloads simply forward to the char const* versions and now use the same const qualifiers, eliminating the earlier inconsistency. Behaviour remains unchanged.


138-160: Implementation consistent with declaration but still inherits attribute problem

The 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.

@Fan-Yunfan Fan-Yunfan changed the title [Fix] Fix const modifier inconsistency in log function declaration/implementation [None][fix] Fix const modifier inconsistency in log function declaration/implementation Aug 7, 2025
@svc-trtllm-gh-bot svc-trtllm-gh-bot added the Community want to contribute PRs initiated from Community label Aug 7, 2025
@Fan-Yunfan
Copy link
Contributor Author

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?

image

@Fan-Yunfan
Copy link
Contributor Author

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.

image

@tongyuantongyu
Copy link
Member

/bot run

@tensorrt-cicd
Copy link
Collaborator

PR_Github #15688 [ run ] triggered by Bot

@tensorrt-cicd
Copy link
Collaborator

PR_Github #15688 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #11800 completed with status: 'FAILURE'

@tongyuantongyu
Copy link
Member

/bot run

@tensorrt-cicd
Copy link
Collaborator

PR_Github #15771 [ run ] triggered by Bot

@tensorrt-cicd
Copy link
Collaborator

PR_Github #15771 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #11856 completed with status: 'FAILURE'

@tongyuantongyu
Copy link
Member

/bot run

@tensorrt-cicd
Copy link
Collaborator

PR_Github #15843 [ run ] triggered by Bot

@tensorrt-cicd
Copy link
Collaborator

PR_Github #15843 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #11909 completed with status: 'SUCCESS'

@tongyuantongyu tongyuantongyu merged commit 41ff490 into NVIDIA:main Aug 21, 2025
4 checks passed
zhou-yuxin pushed a commit to zhou-yuxin/TensorRT-LLM that referenced this pull request Aug 21, 2025
…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>
@Fan-Yunfan
Copy link
Contributor Author

@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.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community want to contribute PRs initiated from Community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants