llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.25k stars 11.67k forks source link

[clang-tidy] misc-include-cleaner incompatible with modernize-deprecated-headers ? #76567

Open Zitrax opened 9 months ago

Zitrax commented 9 months ago

I have not found any way to fix code like this without suppressing with NOLINT or similar.

#include <iostream>

// Including this triggers:
//   inclusion of deprecated C++ header 'string.h'; consider using 'cstring' instead [modernize-deprecated-headers]
#include <string.h>

// Including this instead triggers:
//   no header providing "strsignal" is directly included [misc-include-cleaner]
#include <cstring>

int main() {
    std::cout << strsignal(2) << "\n";
    return 0;
}

The warning from modernize-deprecated-headers makes sense but shouldn't misc-include-headers allow using the modern C++ headers like cstring here without warning?

Godbolt example: https://godbolt.org/z/5TxEvK3P6

Tested with clang-tidy 17.0.6 and 18.0.0

llvmbot commented 9 months ago

@llvm/issue-subscribers-clang-include-cleaner

Author: Daniel Bengtsson (Zitrax)

I have not found any way to fix code like this without suppressing with `NOLINT` or similar. ```cpp #include <iostream> // Including this triggers: // inclusion of deprecated C++ header 'string.h'; consider using 'cstring' instead [modernize-deprecated-headers] #include <string.h> // Including this instead triggers: // no header providing "strsignal" is directly included [misc-include-cleaner] #include <cstring> int main() { std::cout << strsignal(2) << "\n"; return 0; } ``` The warning from `modernize-deprecated-headers` makes sense but shouldn't `misc-include-headers` allow using the modern C++ headers like `cstring` here without warning? Godbolt example: https://godbolt.org/z/5TxEvK3P6 Tested with clang-tidy 17.0.6 and 18.0.0
pkasting commented 9 months ago

strsignal() is POSIX, not standard C, right? Is guaranteed to provide all the symbols of the platform's string.h, even ones not in the C spec? If it is, I don't think it's guaranteed to export them out of namespace std?

All this is my way of saying, I think in this case you want // NOLINT, and the misc-include-headers warning is correct.

Zitrax commented 9 months ago

I think you are right about that.

Still it would be nice if these two checks could co-exist without littering the code with suppressions. cstring includes string.h directly without any protection.