fix(IWYU): force std:: or <> for max, min, copy #411
Conversation
suppress false positive on direct initialization (`int max(0);`)
There was a problem hiding this comment.
Pull request overview
This PR fixes a false positive in the Include What You Use (IWYU) checker by preventing direct initialization syntax (like int max(0);) from being incorrectly flagged as requiring #include <algorithm>. The fix moves copy, max, and min from the general template detection pattern to a specialized pattern that only matches when these functions are:
- Explicitly qualified with
std::(e.g.,std::max(a, b)), OR - Used with template arguments (e.g.,
max<int>(a, b))
Key Changes
- Removes
copy,max, andminfrom the_HEADERS_MAYBE_TEMPLATEStuple to prevent the generic pattern from matching them - Adds a specialized regex pattern that only matches fully qualified or explicitly templated uses of these three functions
- Adds a test case to verify that direct initialization syntax doesn't trigger false positives
- Minor comment improvement for consistency
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cpplint.py | Removes copy, max, min from generic template list and adds specialized pattern with stricter matching rules to avoid false positives |
| cpplint_unittest.py | Adds test case verifying that direct initialization syntax (int max(0)) doesn't trigger IWYU warnings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """, | ||
| "Add #include <algorithm> for min [build/include_what_you_use] [4]", | ||
| ) | ||
| self.TestIncludeWhatYouUse("int max(0), copy(max), min();", "") |
There was a problem hiding this comment.
The test case verifies that false positives are suppressed, but there's no corresponding test to verify that legitimate uses of std::max(), std::min(), or std::copy() still trigger the include warning. Consider adding test cases like:
self.TestIncludeWhatYouUse("int x = std::max(a, b);",
"Add #include <algorithm> for max [build/include_what_you_use] [4]")
self.TestIncludeWhatYouUse("std::copy(src.begin(), src.end(), dst.begin());",
"Add #include <algorithm> for copy [build/include_what_you_use] [4]")This ensures the fix doesn't break detection of actual usage.
There was a problem hiding this comment.
How was this resolved? Do those tests already exist or are they not worth adding?
There was a problem hiding this comment.
Indeed, I didn't think they were worth adding—the max and copy regexes are the same as the already-tested one for min, and just like how regexes for max and copy weren't already there, throughout the unit tests each part of the relevant regex pattern is only tested once. Sorry for resolving without leaving this comment.
|
Thanks—had to search up what a RS is :) Could you also check out #376? That's the one I really want to get over with. |
suppress false positive on direct initialization (
int max(0);)Fixes #296