[None][fix] Fix the incorrect header file import in dataType.h #7133
Conversation
…rsion Signed-off-by: fanyunfan <2569548856@qq.com>
Signed-off-by: fanyunfan <2569548856@qq.com>
📝 WalkthroughWalkthroughUpdated a single include in cpp/include/tensorrt_llm/common/dataType.h to reference tensorrt_llm/common/tllmException.h instead of tensorrt_llm/common/logger.h. No other code or public declarations changed. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cpp/include/tensorrt_llm/common/dataType.h (4)
20-22: Stop relying on transitive includes; add missing headers and remove unused .The file uses size_t, std::string, and std::runtime_error but does not include their owning headers. Add , , and , and drop the unused to make the header self-sufficient. If you adopt the later suggestion to replace std::runtime_error with TLLM_THROW, can be omitted.
Apply:
#include "tensorrt_llm/common/tllmException.h" -#include <NvInferRuntime.h> -#include <map> +#include <NvInferRuntime.h> +#include <cstddef> // size_t +#include <stdexcept> // std::runtime_error +#include <string> // std::string
17-17: Add include guards per repo guideline (keep #pragma once if you like).Coding guidelines require header guards named TRTLLM__H. Add them to avoid multiple inclusion and to align with the standard across the tree.
Apply:
-#pragma once +#ifndef TRTLLM_DATA_TYPE_H +#define TRTLLM_DATA_TYPE_H +#pragma once @@ } // namespace tensorrt_llm::common + +#endif // TRTLLM_DATA_TYPE_HAlso applies to: 87-87
2-2: Consider updating the copyright year to 2025.This file is being modified in 2025; project guidelines state using the current year in headers. If the repo uses a central script for this, feel free to ignore and let the script run.
- * Copyright (c) 1993-2023, NVIDIA CORPORATION. All rights reserved. + * Copyright (c) 1993-2025, NVIDIA CORPORATION. All rights reserved.
81-81: Prefer TLLM_THROW over throwing std::runtime_error directly for consistency.Keeps error handling uniform with the rest of this header and leverages the same formatting/exception type as other branches.
- default: throw std::runtime_error("Unsupported data type"); break; + default: TLLM_THROW("Unsupported data type"); break;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cpp/include/tensorrt_llm/common/dataType.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh}: In C++, close namespaces with a comment naming the namespace (e.g., } // namespace foo)
Prefer const/constexpr variables over #define for constants
Declare variables const if not modified after initialization
Use Allman brace style in C++
C++ filenames use lowerCamelCase and must be case-insensitively unique within a build target
C++ type names use UpperCamelCase
Local variables, methods, and namespaces use lowerCamelCase
Global non-static variables not in anonymous namespace use gPrefix lowerCamelCase (e.g., gExample)
Static globals or globals in anonymous namespaces use sPrefix lowerCamelCase
Locally visible static variables start with 's' (e.g., static std::once_flag sFlag;)
Member variables use mPrefix lowerCamelCase; public members may omit but are encouraged to use 'm'
Constants (enums, global/static/function-scope magic numbers) use kPREFIXED_UPPER_SNAKE (e.g., kDIGIT_NUM)
If macros are unavoidable, use UPPER_SNAKE_CASE (prefer constants over #define)
Constructor parameter that conflicts with a public member name gets trailing underscore (foo_)
Literal suffixes should be uppercase (e.g., 1234L not 1234l)
C++: use spaces only; indent 4 spaces
Run clang-format (LLVM style) before submitting; wrap lines at 120 characters
If formatting must be bypassed, use // clang-format off/on around the section
Prefer smart pointers; use unique_ptr for sole ownership, shared_ptr for shared; weak_ptr only in exceptional cases
Do not use deprecated pre-C++11 smart pointers
Use C++ style comments; avoid C comments except special inline cases; prefer // single-line
Capitalize and punctuate full-sentence comments
Follow Doxygen rules: use //! for comments and //!< for members in C++
Disable code with #if/#endif and mnemonic conditions; avoid commented-out code; avoid dead code
Do not throw exceptions across library boundaries
Use least-forceful casts; avoid removing const/volatile; avoid C-style and functional casts (except constructors); p...
Files:
cpp/include/tensorrt_llm/common/dataType.h
**/*.{h,hpp,hxx,hh,cuh,cpp,cxx,cc,cu}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Parameter names must be consistent between declarations and definitions
Files:
cpp/include/tensorrt_llm/common/dataType.h
**/*.{h,hpp,hxx,hh,cuh}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Header files must use include guards named TRTLLM__H without underscores prefix/suffix (e.g., TRTLLM_FOO_BAR_HELLO_H)
Files:
cpp/include/tensorrt_llm/common/dataType.h
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
cpp/include/tensorrt_llm/common/dataType.h
🧠 Learnings (1)
📓 Common learnings
Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
⏰ 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 (1)
cpp/include/tensorrt_llm/common/dataType.h (1)
19-19: Directly including tllmException.h is the correct fix.This header uses TLLM_THROW and does not rely on any logging facilities, so removing the transitive dependency on logger.h tightens coupling and avoids fragile includes. Looks good.
svc-trtllm-gh-bot
added
the
Community want to contribute
|
@Fan-Yunfan I'll review the changes in your PR and the fix you've implemented in commit 1ece984. ✅ Actions performedReview triggered.
|
|
@coderabbitai review I pushed a fix in commit a8c1956, please review it. |
🧩 Analysis chain@Fan-Yunfan I've reviewed your fix in commit a8c1956. The change is correct and well-reasoned: ✅ What the fix addresses:
✅ Benefits of this change:
✅ Code review:
This is a solid improvement that eliminates a potential maintenance hazard. The fix is ready to merge! 🐰✨ 🏁 Script executed: Length of output: 3249 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 265669 🏁 Script executed: Length of output: 483 🏁 Script executed: Length of output: 754 🏁 Script executed: Length of output: 740 @Fan-Yunfan I've reviewed your fix in commit a8c1956. The change is correct and well-reasoned: ✅ What the fix addresses:
✅ Benefits of this change:
✅ Code review:
This is a solid improvement that eliminates a potential maintenance hazard. The fix is ready to merge! 🐰✨ ✅ Actions performedComments resolved. |
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand.
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
@EmmaQiaoCh Hello～ I noticed that this PR involves code from your previous contributions. Could you please take a look when you have some free time? Thank you! ～ |
|
@tongyuantongyu, |
I don't mind making this change in a larger PR, but I feel it's too trivial on its own and not worth a dedicate PR. |
|
@Fan-Yunfan , thanks for your contribution! What makes things complete are these small changes anyway~ |
|
/bot run |
|
PR_Github #18404 [ run ] triggered by Bot |
|
PR_Github #18404 [ run ] completed with state |
Thank you for your kindness 🌹, and I apologize for my previous misunderstanding. After your reminder, I’ve realized that each CI pipeline run does indeed take a significant amount of time—around 8 hours per check. Combining multiple minor code changes into a single PR to trigger the CI pipeline only once is also a common practice in many engineering optimization strategies. Next time, if I have similar small or incremental code changes, I will consolidate them into one PR to avoid repeated CI pipeline runs and save time. Your kindness and encouragement are also a great motivation that supports my continued contributions to open source. Thank you once again! 🌹🌹🌹 |
|
Thank you for your understanding and contributions! |
…A#7133) 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> Co-authored-by: Kanghwan <861393+karljang@users.noreply.github.com>
…A#7133) 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> Co-authored-by: Kanghwan <861393+karljang@users.noreply.github.com>
5 participants
Problem
Today, while reading the
dataType.hfile, I noticed that the methods defined in this file useTLLM_THROWandthrow std::runtime_errorto handle cases where the input is an unknownnvinfer1::DataType.To use these two exception handling functions, the header file
tensorrt_llm/common/logger.his included. However, theTLLM_THROWmacro are actually defined intensorrt_llm/common/tllmException.h. The reason no compilation error occurred is that thelogger.hincludes theassert.h, which in turn includestllmException.h.Given that the
dataType.hfile does not use any logging-related macros or functions fromtensorrt_llm/common/logger.h, I believe it would be more intuitive and reasonable to replace the included header fromlogger.htotllmException.h.This would avoid ambiguity and potential issues caused by indirect inclusion.
Current Implementation
cpp/include/tensorrt_llm/common/dataType.hSolution
Replace
#include "tensorrt_llm/common/logger.h"with#include "tensorrt_llm/common/tllmException.h".Additional info
tllmException.himport<stdexcept>as same, so don't worrythrow std::runtime_errorwill occur compilation error.