llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

modernize-use-transparent-functors not being fixed

Open sweemer opened this issue 1 year ago • 7 comments

The modernize-use-transparent-functors check offers a fix, and while clang-tidy correctly warns about this check, it does not actually appear to be fixing it for me.

As you can see in the example below, only the modernize-use-trailing-return-type check is actually being fixed. Why isn't modernize-use-transparent-functors being fixed as well?

$ clang-tidy-15 --version
Ubuntu LLVM version 15.0.0

  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: skylake
$ cat test.cpp
#include <functional>
int main() {
  std::plus<int>{}(1, 1);
}
$ clang-tidy-15 -checks=modernize-* -fix test.cpp
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "test.cpp"
No compilation database found in /home/sweemer/test or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
1124 warnings generated.
/home/sweemer/test/test.cpp:2:5: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
int main() {
~~~ ^
auto       -> int
/home/sweemer/test/test.cpp:2:1: note: FIX-IT applied suggested code changes
int main() {
^
/home/sweemer/test/test.cpp:2:11: note: FIX-IT applied suggested code changes
int main() {
          ^
/home/sweemer/test/test.cpp:3:3: warning: prefer transparent functors 'plus<>' [modernize-use-transparent-functors]
  std::plus<int>{}(1, 1);
  ^
clang-tidy applied 2 of 2 suggested fixes.
Suppressed 1122 warnings (1122 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
$ cat test.cpp
#include <functional>
auto main() -> int {
  std::plus<int>{}(1, 1);
}

sweemer avatar Jul 24 '22 07:07 sweemer

@llvm/issue-subscribers-clang-tidy

llvmbot avatar Jul 24 '22 13:07 llvmbot

@sweemer, check does not offer a fix for constructed non-transparent functors:

https://github.com/llvm/llvm-project/blob/eaf0aa1f1fbd06fbd66417f2c9d50d3074969824/clang-tools-extra/clang-tidy/modernize/UseTransparentFunctorsCheck.cpp#L82-L86

However, documentation isn't lying. That check offers a fix, but for other case:

https://github.com/llvm/llvm-project/blob/eaf0aa1f1fbd06fbd66417f2c9d50d3074969824/clang-tools-extra/clang-tidy/modernize/UseTransparentFunctorsCheck.cpp#L121-L123

eoan-ermine avatar Aug 13 '22 18:08 eoan-ermine

Thanks for helping to look into this. Do you know why the fix is only provided for one case and not the other? Since clang-tidy warns about both cases, I think it would be desirable if it fixed both of them too.

sweemer avatar Aug 13 '22 23:08 sweemer

Thanks for helping to look into this. Do you know why the fix is only provided for one case and not the other? Since clang-tidy warns about both cases, I think it would be desirable if it fixed both of them too.

Comment above the matcher for constructed non-transparent functors says:

https://github.com/llvm/llvm-project/blob/eaf0aa1f1fbd06fbd66417f2c9d50d3074969824/clang-tools-extra/clang-tidy/modernize/UseTransparentFunctorsCheck.cpp#L58-L64

It needs to be investigated whether the case char* vs string is the only such case for which a hint cannot be easily provided or not

eoan-ermine avatar Aug 14 '22 12:08 eoan-ermine

It needs to be investigated whether the case char* vs string is the only such case for which a hint cannot be easily provided or not

Could you help explain how the “const char* vs string” problem affects the check and the fix differently?

In other words, if this problem prevents clang-tidy from offering a fix, then how does it not prevent clang-tidy from offering the check for the same code?

Or, are both the check and the fix equally vulnerable to the problem but the check goes ahead and warns about it anyway?

sweemer avatar Aug 15 '22 00:08 sweemer

It needs to be investigated whether the case char* vs string is the only such case for which a hint cannot be easily provided or not

Could you help explain how the “const char* vs string” problem affects the check and the fix differently?

In other words, if this problem prevents clang-tidy from offering a fix, then how does it not prevent clang-tidy from offering the check for the same code?

Or, are both the check and the fix equally vulnerable to the problem but the check goes ahead and warns about it anyway?

Yeah, the check warns about all non-transparent functors. It's ambiguous whether they're meant to preserve required semantics or not.

eoan-ermine avatar Aug 15 '22 19:08 eoan-ermine

Yeah, the check warns about all non-transparent functors. It's ambiguous whether they're meant to preserve required semantics or not.

Just to make sure I understand, in the case where the non-transparent functor is required to preserve semantics, then either the code should be refactored to provide the same result using transparent functors, or the warning should be suppressed with NOLINT, right?

Also, is there any relationship between the SafeMode option and the "const char* vs string" problem?

In other words, if SafeMode is set to true, then would it be technically possible for clang-tidy to fix more cases of this check than it does now?

sweemer avatar Aug 16 '22 00:08 sweemer