Hi Dave, >>Direct inclusion creates ordering requirements which are at least as annoying as unnecessary build dependencies.
I had intended to write back to the list on the last note, anyway, my last note was written before I realized what you were most likely thinking of when you mentioned ordering requirements. Example: I have a .c file that has in it: uword i, j = 10; i = min_log2(j); So I could just include clib.h, as clib.h includes types.h. But I should, according to the pros, (redundantly) include types.h and clib.h. The theory is that if somewhere down the road clib.h no longer needed to include types.h (yes, this is a really poor example), I'd still have types.h included in my .c file needed for the typedef of uword. We are not talking about the old system where the .h files do not include other .h files and therefore the .c file needs all the .h files in correct order, when we say direct includes. We are talking about redundant direct includes. And I'm saying that in my methodology, I am willing to only include clib.h in my .c file, and take a chance that clib.h does not lose types.h as time goes by. Burt On Sat, Sep 23, 2017 at 1:21 PM, Burt Silverman <bur...@gmail.com> wrote: > Hi Dave, > > Indeed, I will not boil the ocean with any patches. The beauty of what I > found is that an individual developer who has the interest can use the > techniques on his own files. That will greatly limit any risk to other > files. Sadly, I have not developed any code, but that also means there > won't be any patches coming from here. > > A couple of technical items: > > 1. For someone doing the full/direct include list, rather than the short > list I tend to prefer, then suppose you would rather see #include > <vlib/vlib.h> rather than individual files included in vlib.h spelled out. > Technically to make this happen you can add to vlib.h: > > // IWYU pragma: begin_exports > > #include yadayada > #include yadayadayada > ... > // IWYU pragma: end_exports > > but it might not work reliably without extra effort so that the yada files > do not sneak through from other places. > > 2. In my example, I moved a few files from builtin_client.h to > builtin_client.c to satisfy the IWYU program. But I thought that the > program treated the corresponding .h file (builtin_client.h) special, so > the IWYU would allow one to add the #includes to either the .h or .c file > without spewing complaints. This one I have to go back and review to > understand it better. > > Burt > > On Sat, Sep 23, 2017 at 9:18 AM, Dave Barach (dbarach) <dbar...@cisco.com> > wrote: > >> 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
