The source for the changes in this PR is the manual page for [`system_data_types(7)`](https://man7.org/linux/man-pages/man3/clockid_t.3.html), which I wrote. That manual page has been written comparing the source code of glibc and the C and POSIX standard documents.
As I add more types to that manual page, I'll send you more PRs of this kind.
About the commit messages:
I tried to follow some conventions in these commit messages so that it is easier to review:
"Add *more* mappings ..." is for commits that add mappings to an already supported type.
"Add mappings... " is for commits adding a type.
"Remove mappings ..." removes incorrect mappings".
"Fix mappings ..." is a mix of the above.
About the commit messages:
I tried to follow some conventions in these commit messages so that it is easier to review:
"Add **more** mappings ..." is for commits that add mappings to an already supported type.
About the commit messages:
I tried to follow some conventions in these commit messages so that it is easier to review:
"Add **more** mappings ..." is for commits that add mappings to an already supported type.
"Add mappings... " is for commits adding a type.
"Removeincorrect mappings ..." removes incorrect mappings (according to the relevant standards).
About the commit messages:
I tried to follow some conventions in these commit messages so that it is easier to review:
"Add **more** mappings ..." is for commits that add mappings to an already supported type.
"Add mappings... " is for commits adding a type.
"Remove incorrect mappings ..." removes incorrect mappings (according to the relevant standards).
I'm unfortunately without computer for a while, so I'll circle back to this when I've managed to get a hold of a new machine. Thanks for the extensive work!
Oh la la! Quite a bit of changes here... Could you rebase on latest master to fix the (unrelated) build error? That way we can see if the test suite is still happy with these mapping changes. Thanks!
Okay, expect that for tonight :-)
I've got a few more mappings, this time about macros. Expect a new PR just after this one too ;)
IWYU seems to be unhappy with conflicting mappings for `sys/socket.h` (see test results)
Hmmm, I these are the differences for `<sys/socket.h>`:
```
$ git diff iwyu/master..alx/master --color | grep sys/socket
+ { symbol: [ "sockaddr", private, "<sys/socket.h>", private ] },
+ { symbol: [ "socklen_t", private, "<sys/socket.h>", private ] },
+ { symbol: [ "ssize_t", private, "<sys/socket.h>", public ] },
+ { symbol: [ "size_t", private, "<sys/socket.h>", public ] },
+ { "sockaddr", kPrivate, "<sys/socket.h>", kPrivate },
+ { "socklen_t", kPrivate, "<sys/socket.h>", kPrivate },
+ { "ssize_t", kPrivate, "<sys/socket.h>", kPublic },
+ { "size_t", kPrivate, "<sys/socket.h>", kPublic },
```
I checked that these are correct, and I found a mistake in the manual page about `ssize_t` (the types are required by POSIX.1-2008 but not by POSIX.1-2001). But I doubt that is the reason... I'll check the source code of the test.
Hmmm, I these are the differences for `<sys/socket.h>`:
```
$ git diff iwyu/master..alx/master --color | grep sys/socket
+ { symbol: [ "sockaddr", private, "<sys/socket.h>", private ] },
+ { symbol: [ "socklen_t", private, "<sys/socket.h>", private ] },
+ { symbol: [ "ssize_t", private, "<sys/socket.h>", public ] },
+ { symbol: [ "size_t", private, "<sys/socket.h>", public ] },
+ { "sockaddr", kPrivate, "<sys/socket.h>", kPrivate },
+ { "socklen_t", kPrivate, "<sys/socket.h>", kPrivate },
+ { "ssize_t", kPrivate, "<sys/socket.h>", kPublic },
+ { "size_t", kPrivate, "<sys/socket.h>", kPublic },
```
I checked that these are correct, and I found a mistake in the manual page about `ssize_t` (the types are required in `<aio.h>` and `<sys/socket.h>` by POSIX.1-2008 but not by POSIX.1-2001). But I doubt that is the reason... I'll check the source code of the test.
Hmmm, I these are the differences for `<sys/socket.h>`:
```
$ git diff iwyu/master..alx/master --color | grep sys/socket
+ { symbol: [ "sockaddr", private, "<sys/socket.h>", private ] },
+ { symbol: [ "socklen_t", private, "<sys/socket.h>", private ] },
+ { symbol: [ "ssize_t", private, "<sys/socket.h>", public ] },
+ { symbol: [ "size_t", private, "<sys/socket.h>", public ] },
+ { "sockaddr", kPrivate, "<sys/socket.h>", kPrivate },
+ { "socklen_t", kPrivate, "<sys/socket.h>", kPrivate },
+ { "ssize_t", kPrivate, "<sys/socket.h>", kPublic },
+ { "size_t", kPrivate, "<sys/socket.h>", kPublic },
```
I checked that these are correct, and I found a mistake in the manual page about `ssize_t` (the types are required in `<aio.h>`, `<mqueue.h>`, and `<sys/socket.h>` by POSIX.1-2008 but not by POSIX.1-2001). But I doubt that is the reason... I'll check the source code of the test.
IWYU has a few mechanisms for auto-mapping as well (based on comments and pragmas in libc headers, for example). I wonder if the new mappings somehow conflict with those.
Hmm, that's probably the case. Do you know if/how we could fix that? Maybe disable it for identifiers that are explicitly mapped?
I grepped `size_t` and other already mapped types to see if there was some exception for them, but I couldn't find anything. Why did it start failing now?
Oops. Clicked the wrong button
Maybe the assertion is misguided, not sure. I think you can run with higher verbose level to see some of the mappings unfold, e.g. `IWYU_VERBOSE=6 ./run_iwyu_tests.py sometestname`
I couldn't manage to build this project. I get a bunch of errors (due to dependencies). Do you have a full list of dependencies for debian (preferably Sid) or ubuntu (preferably Focal)?
Or if you can test it, could you show the logs here?
Thanks.
I installed a fresh ubuntu focal just now, and I _think_ the only packages I added were inspired by https://github.com/include-what-you-use/include-what-you-use/blob/master/.github/workflows/ci.yml#L23.
That is, you need to add https://apt.llvm.org as a source and then install `llvm-13-dev`, `libclang-13-dev` and `clang-13` (might work without `-13` marker as well). That should be enough for the build instructions in https://github.com/include-what-you-use/include-what-you-use/blob/master/README.md#how-to-build to work.
Ahh, yes, I was missing `libclang-dev`
A few news:
- I tried removing the 8 lines involving `<sys/socket.h>`, and then only 1 test fails.
- The verbose level doesn't change the output at all (eventhough it is correctly recognized).
I'll add a commit to remove the sys/socket lines and we can see the new test results.
A few news:
- I tried removing the 8 lines involving `<sys/socket.h>`, and then only 1 test fails.
- The verbose level doesn't change the output at all (even though it is correctly recognized).
A few news:
- I tried removing the 8 lines involving `<sys/socket.h>`, and then only 1 test fails.
- The verbose level doesn't change the output at all (even though it is correctly recognized).
I'll add a commit to remove the `<sys/socket.h>` lines and we can see the new test results.
Now this is failing:
```
105/139 Test #14: cxx.test_badinc ...................................***Failed 6.03 sec
INFO:root:Testing iwyu on tests/cxx/badinc.cc
>>> Running /home/runner/work/include-what-you-use/include-what-you-use/build/bin/include-what-you-use -Xiwyu --verbose=3 -Xiwyu --mapping_file=tests/cxx/badinc.imp -I . tests/cxx/badinc.cc
(tests/cxx/badinc-inl.h has correct #includes/fwd-decls)
tests/cxx/badinc.h:64:7: warning: I2_Class is defined in "tests/cxx/badinc-i2.h", which isn't directly #included.
...
tests/cxx/badinc.h:374:27: warning: I2_TemplateClass::~I2_TemplateClass<FOO> is defined in "tests/cxx/badinc-i2-inl.h", which isn't directly #included.
tests/cxx/badinc.h should add these lines:
#include <stdio.h> // for printf, NULL
#include <set> // for set
#include <vector> // for vector
#include "tests/cxx/badinc-i2-inl.h" // for I2_Class::~I2_Class, I2_TemplateClass::~I2_TemplateClass<FOO>, I2_Class::I2_Class, I2_Class::InlFileFn, I2_Class::InlFileStaticFn, I2_Class::InlFileTemplateFn, I2_TemplateClass::I2_TemplateClass<FOO>, I2_TemplateClass::InlFileTemplateClassFn
#include "tests/cxx/badinc-i2.h" // for I2_Enum, I2_Class, I2_Struct, I21, I2_EnumForTypedefs, I22, I2_TemplateClass, I2_Typedef, TemplateForHClassTplFn (ptr only), I2_MACRO, I2_TypedefOnly_Class (ptr only)
tests/cxx/badinc.h should remove these lines:
- #include <ctype.h> // lines 14-14
- #include <math.h> // lines 16-16
- #include "tests/cxx/badinc-d2.h" // lines 19-19
- class H_ForwardDeclareClass; // lines 22-22
- template <typename T> class I2_TypedefOnly_Class; // lines 28-28
The full include-list for tests/cxx/badinc.h:
#include <errno.h> // for errno
#include <stdio.h> // for printf, NULL
#include <queue> // for queue
#include <set> // for set
#include <string> // for string
#include <vector> // for vector
#include "tests/cxx/badinc-d3.h" // for D3_Enum, D31
#include "tests/cxx/badinc-i2-inl.h" // for I2_Class::~I2_Class, I2_TemplateClass::~I2_TemplateClass<FOO>, I2_Class::I2_Class, I2_Class::InlFileFn, I2_Class::InlFileStaticFn, I2_Class::InlFileTemplateFn, I2_TemplateClass::I2_TemplateClass<FOO>, I2_TemplateClass::InlFileTemplateClassFn
#include "tests/cxx/badinc-i2.h" // for I2_Enum, I2_Class, I2_Struct, I21, I2_EnumForTypedefs, I22, I2_TemplateClass, I2_Typedef, TemplateForHClassTplFn (ptr only), I2_MACRO, I2_TypedefOnly_Class (ptr only)
class Cc_Class; // lines 25-25
class Cc_Struct; // lines 24-24
class H_Class::H_Class_DefinedInI1; // lines 107-107
class H_Class::H_Class_Subdecl; // lines 105-105
class H_Class::H_Class_UnusedSubdecl; // lines 106-106
template <typename T> class H_ScopedPtr; // lines 34-34
---
tests/cxx/badinc.cc:81:35: warning: MACRO_CALLING_I6_FUNCTION is defined in "tests/cxx/badinc-i1.h", which isn't directly #included.
...
tests/cxx/badinc.cc:1845:22: warning: kI1ConstInt is defined in "tests/cxx/badinc-i1.h", which isn't directly #included.
tests/cxx/badinc.cc should add these lines:
#include <ctype.h> // for isascii
#include <stddef.h> // for offsetof
#include <list> // for list
#include "tests/cxx/badinc-i1.h" // for I1_Class, I1_TemplateClass, I1_Enum, I1_ClassPtr, I1_TemplateMethodOnlyClass, I1_TemplateFunction, I1_const_ptr, I1_Struct, kI1ConstInt, I11, I1_Function, i1_GlobalFunction, operator==, I12, OperateOn, I1_MACRO_SYMBOL_WITHOUT_VALUE, I1_NamespaceClass, I1_MACRO_SYMBOL_WITH_VALUE, I1_MACRO_SYMBOL_WITH_VALUE2, I1_MemberPtr, I1_Union, I1_UnnamedStruct, I2_OperatorDefinedInI1Class::operator<<, EmptyDestructorClass, I13, I1_And_I2_OverloadedFunction, I1_Base, I1_Class::NestedStruct, I1_FunctionPtr, I1_NamespaceTemplateFn, I1_Typedef, I1_TypedefOnly_Class, MACRO_CALLING_I6_FUNCTION, H_Class::H_Class_DefinedInI1, I1_I2_Class_Typedef, I1_MACRO_LOGGING_CLASS, I1_MACRO_SYMBOL_WITH_VALUE0, I1_ManyPtrStruct (ptr only), I1_NamespaceStruct, I1_OverloadedFunction, I1_PtrAndUseOnSameLine, I1_PtrDereferenceClass, I1_PtrDereferenceStatic, I1_PtrDereferenceStruct, I1_SiblingClass, I1_StaticMethod, I1_Subclass, I1_SubclassesI2Class, I1_TemplateClass<>::I1_TemplateClass_int, I1_TemplateClassFwdDeclaredInD2 (ptr only), I1_TemplateSubclass, I1_TypedefOnly_Class<>::i, I1_UnusedNamespaceStruct (ptr only), i1_int, i1_int_global, i1_int_global2, i1_int_global2sub, i1_int_global3, i1_int_global3sub, i1_int_global4, i1_int_global4sub, i1_int_globalsub, i1_ns2, i1_ns4, i1_ns5
class D2_Class;
class D2_ForwardDeclareClass;
class D2_Subclass;
class I1_ForwardDeclareClass;
namespace d3_namespace { struct D3_Struct; }
namespace i3_ns1 { namespace i3_ns2 { namespace i3_ns3 { template <typename A, int B> struct I3_ForwardDeclareNamespaceTemplateStruct; } } }
namespace i3_ns1 { namespace { struct I3_UnnamedNamespaceStruct; } }
struct I3_ForwardDeclareStruct;
template <typename A, int B, char C> struct I3_ForwardDeclareTemplateStruct;
tests/cxx/badinc.cc should remove these lines:
- #include <math.h> // lines 59-59
- #include <algorithm> // lines 75-75
- #include <algorithm> // lines 76-76
- #include <clocale> // lines 73-73
- #include <locale> // lines 70-70
- #include "tests/cxx/badinc-d2.h" // lines 64-64
- class Cc_ForwardDeclare_Function::I2_Class; // lines 1005-1005
- class I3_UnusedClass; // lines 162-162
- template <class T = I1_Class, I1_Enum E = I11> class Cc_DeclareOnlyTemplateClass; // lines 309-309
The full include-list for tests/cxx/badinc.cc:
#include "tests/cxx/badinc.h"
#include "tests/cxx/badinc-inl.h"
#include <ctype.h> // for isascii
#include <setjmp.h>
#include <stddef.h> // for offsetof
#include <algorithm> // for find
#include <fstream> // for fstream
#include <list> // for list
#include <string> // for basic_string, string, operator+, basic_string<>::iterator
#include <typeinfo> // for type_info
#include "tests/cxx/badinc-d1.h" // for D1_I1_Typedef, D1_Enum, D1CopyClassFn, D1Function, D1_TemplateClass, D1_CopyClass, D1_Subclass, D11, D1_Class, D1_StructPtr, D1_TemplateStructWithDefaultParam, MACRO_CALLING_I4_FUNCTION
#include "tests/cxx/badinc-d4.h" // for operator<<, D4_ClassForOperator
#include "tests/cxx/badinc-i1.h" // for I1_Class, I1_TemplateClass, I1_Enum, I1_ClassPtr, I1_TemplateMethodOnlyClass, I1_TemplateFunction, I1_const_ptr, I1_Struct, kI1ConstInt, I11, I1_Function, i1_GlobalFunction, operator==, I12, OperateOn, I1_MACRO_SYMBOL_WITHOUT_VALUE, I1_NamespaceClass, I1_MACRO_SYMBOL_WITH_VALUE, I1_MACRO_SYMBOL_WITH_VALUE2, I1_MemberPtr, I1_Union, I1_UnnamedStruct, I2_OperatorDefinedInI1Class::operator<<, EmptyDestructorClass, I13, I1_And_I2_OverloadedFunction, I1_Base, I1_Class::NestedStruct, I1_FunctionPtr, I1_NamespaceTemplateFn, I1_Typedef, I1_TypedefOnly_Class, MACRO_CALLING_I6_FUNCTION, H_Class::H_Class_DefinedInI1, I1_I2_Class_Typedef, I1_MACRO_LOGGING_CLASS, I1_MACRO_SYMBOL_WITH_VALUE0, I1_ManyPtrStruct (ptr only), I1_NamespaceStruct, I1_OverloadedFunction, I1_PtrAndUseOnSameLine, I1_PtrDereferenceClass, I1_PtrDereferenceStatic, I1_PtrDereferenceStruct, I1_SiblingClass, I1_StaticMethod, I1_Subclass, I1_SubclassesI2Class, I1_TemplateClass<>::I1_TemplateClass_int, I1_TemplateClassFwdDeclaredInD2 (ptr only), I1_TemplateSubclass, I1_TypedefOnly_Class<>::i, I1_UnusedNamespaceStruct (ptr only), i1_int, i1_int_global, i1_int_global2, i1_int_global2sub, i1_int_global3, i1_int_global3sub, i1_int_global4, i1_int_global4sub, i1_int_globalsub, i1_ns2, i1_ns4, i1_ns5
#include "tests/cxx/badinc2.c"
class D2_Class;
class D2_ForwardDeclareClass;
class D2_Subclass;
class ForwardDeclareOnlyClass; // lines 167-167
class ForwardDeclareOnlyForTypedefClass; // lines 172-172
class I1_ForwardDeclareClass;
class I3_ForwardDeclareClass; // lines 159-159
class MacroClass; // lines 170-170
namespace d3_namespace { struct D3_Struct; }
namespace i3_ns1 { namespace i3_ns2 { namespace i3_ns3 { struct I3_ForwardDeclareNamespaceStruct; } } } // lines 164-164
namespace i3_ns1 { namespace i3_ns2 { namespace i3_ns3 { template <typename A, int B> struct I3_ForwardDeclareNamespaceTemplateStruct; } } }
namespace i3_ns1 { namespace { struct I3_UnnamedNamespaceStruct; } }
struct Cc_C_Struct; // lines 182-182
struct I3_ForwardDeclareStruct;
template <class T> struct Cc_OnlySpecializedStruct; // lines 177-177
template <typename A, int B, char C> struct I3_ForwardDeclareTemplateStruct;
template <typename T> struct I3_SimpleForwardDeclareTemplateStruct; // lines 160-161
---
Traceback (most recent call last):
File "run_iwyu_tests.py", line 127, in <module>
exit(TestIwyuOnRelevantFiles(runner_args.run_test_file))
File "run_iwyu_tests.py", line 64, in TestIwyuOnRelevantFiles
iwyu_test_util.TestIwyuOnRelativeFile(filename, files_to_check, verbose=True)
File "/home/runner/work/include-what-you-use/include-what-you-use/iwyu_test_util.py", line 476, in TestIwyuOnRelativeFile
raise AssertionError(''.join(failures))
AssertionError:
tests/cxx/badinc.cc:1098: Unexpected diagnostic:
va_list is defined in <stdio.h>, which isn't directly #included.
tests/cxx/badinc.cc:1098: Unmatched regex:
va_list is...*<stdarg.h>
Unexpected summary diffs for tests/cxx/badinc.cc:
+++
@@ -1,6 +1,5 @@
tests/cxx/badinc.cc should add these lines:
#include <ctype.h>
-#include <stdarg.h>
#include <stddef.h>
#include <list>
#include "tests/cxx/badinc-i1.h"
@@ -30,7 +29,6 @@
#include "tests/cxx/badinc-inl.h"
#include <ctype.h> // for isascii
#include <setjmp.h>
-#include <stdarg.h> // for va_list
#include <stddef.h> // for offsetof
#include <algorithm> // for find
#include <fstream> // for fstream
---
```
This test is outdated. Now that we added the information that `va_list` is also defined by `<stdio.h>`, the test should be updated.
Locally it is passing the tests for me.
We have two problems now:
- Investigate why is `<sys/socket.h>` problematic.
- Fix the test about `va_list`.
But that's not urgent.
Locally it is passing the tests for me.
I removed the lines about `va_list` from the test.
We have two problems now:
- Investigate why is `<sys/socket.h>` problematic.
- Fix the test about `va_list` to be able to reintroduce it
@alejandro-colomar Cool!
That test is the original test, so it covers basically everything in a single place. Later developments have added one test per mechanism to avoid cross-pollution.
It feels wrong to go to `<stdio.h>` as a primary candidate for `va_list`, but if `<stdio.h>` is already available in the translation unit and `<stdarg.h>` isn't, it makes more sense. I'll see if I can carve out some time to prove that one way or the other. Unless you beat me to it :-)
I removed the lines about `va_list` from the test.
I removed the lines about `va_list` from the test.
Locally it is passing the tests for me.
We have two problems now:
- Investigate why `<sys/socket.h>` is problematic.
`badinc.cc` includes `<stdio.h>` indirectly, so it's correct that it's suggested for `va_list` after your mapping changes.
And for some reason you've made the `sys/socket.h` symbol mappings private-to-private, where all others are private-to-public: https://github.com/include-what-you-use/include-what-you-use/pull/930/commits/888a91286e18c75ffe4d97b99d1659daaa8e42b8#diff-2ab58a9a3375bbbdb027d23af0a61ae0c5880be9d44ecaa4157b7693124e4f90R135
Changing those to public instead makes the test suite pass without assertions.
Once that's functionally fixed, I can start trying to review the individual commits. Thanks!
>
>
> And for some reason you've made the `sys/socket.h` symbol mappings private-to-private, where all others are private-to-public: [888a912#diff-2ab58a9a3375bbbdb027d23af0a61ae0c5880be9d44ecaa4157b7693124e4f90R135](https://github.com/include-what-you-use/include-what-you-use/commit/888a91286e18c75ffe4d97b99d1659daaa8e42b8#diff-2ab58a9a3375bbbdb027d23af0a61ae0c5880be9d44ecaa4157b7693124e4f90R135)
Good catch!
I guess I replaced `bits/socket` in this line, and then copy&pasted:
```
{ symbol: [ "socklen_t", private, "<bits/socket.h>", private ] },
```
>
> Changing those to public instead makes the test suite pass without assertions.
>
> Once that's functionally fixed, I can start trying to review the individual commits. Thanks!
Great! Let me amend those commits.
Could you please provide a code suggestion for `va_list`? I'm not sure I know what should be done.
> Could you please provide a code suggestion for `va_list`? I'm not sure I know what should be done.
I had something like this in mind:
```diff
diff --git a/tests/cxx/badinc.cc b/tests/cxx/badinc.cc
index e58a98f..24b4a2d 100644
--- a/tests/cxx/badinc.cc
+++ b/tests/cxx/badinc.cc
@@ -1094,8 +1094,10 @@ int main() {
// IWYU: I1_PtrDereferenceClass needs a declaration
I1_PtrDereferenceClass* local_i1_ptrdereference_class = 0;
int x;
- // IWYU: va_list is...*<stdarg.h>
- va_list vl; // in gcc, va_list is an internal type, so this tests <built-in>
+ // va_list is normally in stdarg.h, but we already have stdio.h available,
+ // so mappings will source it from there.
+ // IWYU: va_list is...*<stdio.h>
+ va_list vl;
D1_I1_Typedef d1_i1_typedef;
// IWYU: i1_int is...*badinc-i1.h
int vararray[i1_int];
@@ -1859,7 +1861,6 @@ int main() {
tests/cxx/badinc.cc should add these lines:
#include <ctype.h>
-#include <stdarg.h>
#include <stddef.h>
#include <list>
#include "tests/cxx/badinc-i1.h"
@@ -1889,7 +1890,6 @@ The full include-list for tests/cxx/badinc.cc:
#include "tests/cxx/badinc-inl.h"
#include <ctype.h> // for isascii
#include <setjmp.h>
-#include <stdarg.h> // for va_list
#include <stddef.h> // for offsetof
#include <algorithm> // for find
#include <fstream> // for fstream
```
It fails on master and succeeds on your branch, as far as I can tell.