Dear Burt, This is of interest, but I have concerns about boiling the ocean at this point in the release cycle. Please hold any patches on this topic until well after the 17.10 RC1 throttle branch pull.
Although we haven’t caused ourselves massive pain with similar work - coding standards cleanup, build-related directory refactoring - I’m not convinced that restructuring existing header files is worth the pain it may cause. Direct inclusion creates ordering requirements which are at least as annoying as unnecessary build dependencies. Thanks… Dave From: vpp-dev-boun...@lists.fd.io [mailto:vpp-dev-boun...@lists.fd.io] On Behalf Of Burt Silverman Sent: Friday, September 22, 2017 9:39 PM To: vpp-dev <vpp-dev@lists.fd.io> Subject: [vpp-dev] Practical use of the include-what-you-use tool for individual developers This is a follow up on my recent post about the include-what-you-use tool. I discovered a way that you can use this tool to include a more appropriate set of header files in the files that you develop than would otherwise be the case. The stated philosophy behind the tool is that you should directly include all header files that are used by a file. If struct a is declared in a.h and struct b is declared in b.h, your .c file that references both struct a and struct b should directly include a.h and b.h. But this will lead to including many more files than we have typically done in vpp. My personal preference, although not sanctioned by the pros, is to allow indirect header file inclusion. It turns out that there is a simple way to do this using include-what-you-use, and it does not require rewriting the tool's code. include-what-you-use will suggest which header files should be added and which header files should be removed from the file that you are analyzing. Understand that the corresponding .h file to a .c file will be treated specially. It will be analyzed along with the .c file. For an example, I will use vnet/tcp/builtin_client.c. First I show files that are suggested to be removed from builtin_client.h. vnet/tcp/builtin_client.h should remove these lines: - #include <svm/svm_fifo_segment.h> // lines 28-28 - #include <vnet/ip/ip.h> // lines 22-22 - #include <vnet/session/application_interface.h> // lines 30-30 - #include <vnet/session/session.h> // lines 29-29 - #include <vppinfra/error.h> // lines 26-26 - #include <vppinfra/hash.h> // lines 25-25 Running include-what-you-use involves running the clang C compiler, so if a necessary header file is missing and a type cannot be resolved, you will see regular compiler error messages. After removing the header file includes above from builtin_client.h, and re-running include-what-you-use, we find the error: In file included from vnet/tcp/builtin_client.c:20: ./vnet/tcp/builtin_client.h:40:3: error: unknown type name 'svm_fifo_t' svm_fifo_t *server_rx_fifo; ^ We manually search for the svm_fifo_t declaration and we see that rather than including svm_fifo_segment.h in builtin_client.h, we should have included svm_fifo.h. Fixing that and re-running IWYU, we find vnet/tcp/builtin_client.c:55:3: error: use of undeclared identifier 'session_fifo_event_t' session_fifo_event_t evt; ^ so therefore, session.h should have been included in builtin_client.c rather than builtin_client.h. We also find vnet/tcp/builtin_client.c:258:8: error: use of undeclared identifier 'vnet_disconnect_args_t' vnet_disconnect_args_t _a, *a = &_a; ^ so application_interface.h should have been included in builtin_client.c rather than builtin_client.h. Re-running IWYU tells us that no lines need to be removed from builtin_client.h, however, vnet/tcp/builtin_client.c should remove these lines: - #include <vlibapi/api.h> // lines 24-24 - #include <vlibmemory/api.h> // lines 25-25 - #include <vlibsocket/api.h> // lines 26-26 - #include <vnet/plugin/plugin.h> // lines 19-19 - #include <vnet/vnet.h> // lines 18-18 - #include <vpp/app/version.h> // lines 27-27 Removing these includes, re-running IWYU indicates that no more includes need to be removed from either builtin_client.h or builtin_client.c, and the compilation is successful. We are done, and we have builtin_client.h includes: #include <vnet/vnet.h> #include <vnet/ethernet/ethernet.h> #include <vlibmemory/unix_shared_memory_queue.h> #include <svm/svm_fifo.h> builtin_client.c includes: #include <vnet/tcp/builtin_client.h> #include <vnet/session/session.h> #include <vnet/session/application_interface.h> Now, if on the other hand, a developer prefers to include all the headers directly, like many experts like to see, the result would be: The full include-list for vnet/tcp/builtin_client.c: #include <vnet/tcp/builtin_client.h> #include <string.h> // for memset, NULL #include <vnet/session/application_interface.h> // for vnet_app_attach_args_t #include <vnet/session/session.h> // for stream_session_handle #include "svm/svm_fifo.h" // for svm_fifo_t, svm_fif... #include "vlib/cli.h" // for vlib_cli_output #include "vlib/global_funcs.h" // for vlib_get_thread_main #include "vlib/init.h" // for VLIB_INIT_FUNCTION #include "vlib/node_funcs.h" // for vlib_process_get_ev... #include "vlib/threads.h" // for vlib_get_thread_index #include "vlibapi/api_common.h" // for api_main_t, api_main #include "vlibmemory/api_common.h" // for vl_api_memclnt_crea... #include "vlibmemory/unix_shared_memory_queue.h" // for unix_shared_memory_... #include "vnet/session/application.h" // for session_cb_vft_t #include "vnet/session/stream_session.h" // for stream_session_t #include "vppinfra/clib.h" // for PREDICT_FALSE, ARRA... #include "vppinfra/clib_error.h" // for clib_error_t #include "vppinfra/elog.h" // for ELOG_DATA, ELOG_TYP... #include "vppinfra/error.h" // for clib_warning, clib_... #include "vppinfra/error_bootstrap.h" // for ASSERT #include "vppinfra/format.h" // for unformat, unformat_... #include "vppinfra/memcpy_avx.h" // for clib_memcpy #include "vppinfra/pool.h" // for pool_header_t, pool... #include "vppinfra/smp.h" // for __sync_fetch_and_add_4 #include "vppinfra/vec.h" // for vec_validate, vec_add1 #include "vppinfra/vec_bootstrap.h" // for vec_header_t, vec_len --- and The full include-list for vnet/tcp/builtin_client.h: #include <pthread.h> // for pthread_t #include <svm/svm_fifo.h> // for svm_fifo_t #include <vlibmemory/unix_shared_memory_queue.h> // for unix_shared_memory_... #include <vnet/ethernet/ethernet.h> // for ethernet_main_t #include <vnet/vnet.h> // for vnet_main_t #include <vppinfra/clib.h> // for u32, u64, u8, f64 #include "vlib/main.h" // for vlib_main_t #include "vlib/node.h" // for vlib_node_registrat... #include "vppinfra/lock.h" // for clib_spinlock_t --- Again, the beauty is that a developer can focus on just his or her files, and come up with a short list or a long list of #includes. I will discuss some additional fine points in a future post. In the meantime, note that it is somewhat tricky to get the tool to give you consistent usage of '"' versus '<' and '>'. Possible but a bit tricky or annoying. Roughly it involves having the header files installed under /usr/include, and not using -I. Anyway, here it was only an issue with the long list of #includes. Regards, Burt
_______________________________________________ vpp-dev mailing list vpp-dev@lists.fd.io https://lists.fd.io/mailman/listinfo/vpp-dev
