include-what-you-use / include-what-you-use

A tool for use with clang to analyze #includes in C and C++ source files
https://include-what-you-use.org
Other
4.13k stars 388 forks source link

implementation include suggested for `std::chrono::*` #1277

Closed firewave closed 3 months ago

firewave commented 1 year ago
#include <chrono>

using Clock = std::chrono::steady_clock;
using TimePoint = std::chrono::time_point<Clock>;

void f()
{
    (void)TimePoint::max();
}
input.cpp should add these lines:
#include <bits/chrono.h>  // for steady_clock, time_point
input.cpp should remove these lines:
- #include <chrono>  // lines 1-1

The full include-list for input.cpp:
#include <bits/chrono.h>  // for steady_clock, time_point
---

https://en.cppreference.com/w/cpp/chrono

Happens with 0.20 on Fedora 38 and include-what-you-use 0.18 based on Debian clang version 14.0.6.

kimgr commented 1 year ago

Using libc++ on Mac does not trigger the same behavior, so probably there's some difference between libc++ and libstdc++ here.

kimgr commented 10 months ago

@firewave This appears to be fixed on latest master (now testing on Linux w/ both libstdc++ and libc++). Can you give latest a shot and see what happens?

kimgr commented 10 months ago

Hmm, actually, I tried bisecting to find where it was fixed, and I can't repro on either of 0.17, 0.18, 0.19, 0.20 or mainline. So something is different about my system. I'm on gcc 11.4.0 with the corresponding libstdc++.

kimgr commented 9 months ago

Cannot reproduce on master, so closing. If this shows up again once 0.22 is released, please reopen and we'll see if we can pin down what's different.

firewave commented 9 months ago

Please re-open. It definitely still occurs with 0.21.

I think this is a distro-specific difference. I also encountered the C vs. C++ header (e.g. climits vs. limits.h) difference with the same IWYU version on different distros (I think it was Kali Linux aka Debian vs. Fedora).

Unfortunately I did not look into this as I do not have both distros locally so testing it is quite a pain...

firewave commented 9 months ago

https://github.com/danmar/cppcheck/actions/runs/7770205393

/__w/cppcheck/cppcheck/lib/valueflow.cpp should add these lines:
#include "limits.h"           // for LLONG_MAX, LLONG_MIN
#include <bits/chrono.h>      // for std::chrono::duration, std::chrono::operator+, std::chrono::operator>, std::chrono::time_point, std::chrono::seconds, std::chrono::steady_clock

/__w/cppcheck/cppcheck/lib/valueflow.cpp should remove these lines:
- #include <chrono>  // lines 111-111
- #include <climits>  // lines 112-112

I could extend the job to collect more information (hopefully in a separate log) so I do not need to run it manually.

Maybe the IWYU CI could be extended by using different distros. It might also make sense to run it against libc++ (maybe via AppleClang - two birds, one scorn) as well if it doesn't already. And there is also the MSVC STL - that would be the three major implementations.

firewave commented 9 months ago
      - name: iwyu_tool
        run: |
          PWD=$(pwd)
          iwyu_tool -p cmake.output -j $(nproc) -- -w -Xiwyu --max_line_length=1024 -Xiwyu --comment_style=long -Xiwyu --quoted_includes_first -Xiwyu --update_comments -Xiwyu --mapping_file=$PWD/qt5.imp -I/usr/lib/clang/17/include > iwyu.log

It might be caused by the explicit clang include folder. That is necessary if the IWYU and compiler version run out-of-sync on a distro (though probably not necessary for this distro). It should also be -isystem instead of -I.

kimgr commented 9 months ago

It might be caused by the explicit clang include folder.

Just FYI, that''s not generally safe to do -- the Clang libraries IWYU use depend on a fixed version of the builtin headers, and I'm not sure how well-defined the behavior is if Clang has one idea about a builtin declaration, and the builtin headers have another.

firewave commented 9 months ago

Just FYI, that''s not generally safe to do -- the Clang libraries IWYU use depend on a fixed version of the builtin headers, and I'm not sure how well-defined the behavior is if Clang has one idea about a builtin declaration, and the builtin headers have another.

Good to know. But on some distros you have to do that otherwise IWYU won't work at all because of missing includes. This has been working out fine - so far.

It is quite painful to find a distro with the latest version (which also works) hence the bunny-hopping between them with each release in that aforementioned workflow.

I will give this a closer look in the next few days so this can resolved before the next release - hopefully.

firewave commented 9 months ago

Changing that include to -isystem fixes the climits vs. limits.h issue. But I still get the actual one from the ticket:

/__w/cppcheck/cppcheck/lib/valueflow.cpp should add these lines:
#include <bits/chrono.h>      // for std::chrono::duration, std::chrono::operator+, std::chrono::operator>, std::chrono::time_point, std::chrono::seconds, std::chrono::steady_clock

/__w/cppcheck/cppcheck/lib/valueflow.cpp should remove these lines:
- #include <chrono>  // lines 111-111

What flags do I need again to get to the bottom of it?

firewave commented 9 months ago

Okay - adding the verbose output to the CI doesn't seem like a good idea. I just got a almost 5GiB verbose output from the files this originates from ...

I can reproduce it locally where I still have 0.20 on Manjaro Linux. And the CI is also still using 0.20.

Looking at the pending 0.21 update - that no longer reports it. So this appears indeed to be fixed. Sorry about that confusion.

Icantjuddle commented 7 months ago

I'm seeing this when compiling with clang and libstdc++:

add '#include <bits/chrono.h>' (for std::chrono::duration, std::chrono::duration_cast, std::chrono::high_resolution_clock, std::chrono::operator-, std::chrono::s...)

Using version 0.22.

firewave commented 3 months ago

As it was duplicated by #1552 which is fixed in 0.23 this should be closed as well.

Jackaed commented 2 months ago

Are we certain this was fixed for clang? I'm still experiencing it on master with clang 19 rc3.

kimgr commented 2 months ago

@Jackaed libc++ support is still work in progress

Edit: well, technically, everything is work in progress, but libc++ support is a little behind.

Jackaed commented 2 months ago

Ah, fair enough. Apologies for the comment then.

firewave commented 2 months ago

I cannot reproduce the issue with the original example using 0.20 locally. This needs some looking into.