[PATCH] D144216: [clang-tidy] Extract string header from redundant-string-cstr checker

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D144216#4169781 <https://reviews.llvm.org/D144216#4169781>, @mikecrowe wrote:

> In D144216#4169772 <https://reviews.llvm.org/D144216#4169772>, @carlosgalvezp wrote:
>
>> In D144216#4169764 <https://reviews.llvm.org/D144216#4169764>, @mikecrowe wrote:
>>
>>>> I will double check that this is true once my current build is complete.
>>>
>>> Yes, it's true. I stuck a `#error` in `clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h` and I saw the expected error from a file including `<string>`.
>>
>> Awesome, thanks for checking, I wasn't aware we already had `string.h`. Then if we do have headers named the same as the standard headers, would it make sense to name this header `cstring` instead of `string`?
>
> I think you mean rename `string.h` to `cstring`. (`string` clearly has to be just `string` if it's the standard C++ `string` header.)
>
> A bit of searching in the existing checks shows that `string.h` is included by C checks (`bugprone/signal-handler*.c`) and also by checks that deliberately want `string.h` so they can suggest switching to `cstring` (`modernize/deprecated-headers*`) so it looks like it's necessary to keep `string.h`. If you wish, a `cstring` wrapper that just includes `string.h` could be created, but it really ought to put everything in `namespace std` too. (If so, I think this would probably be better done as a separate change.)

You are absolutely right, I don't know why I mixed `cstring` with `string`, nevermind! Pushed now, will look at the follow up patches soon. Thanks again for the fix!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144216/new/

https://reviews.llvm.org/D144216