On Fri, 2022-07-29 at 11:12 +0100, Quentin Monnet wrote: > On 28/07/2022 19:25, Yonghong Song wrote: > > > > > > On 7/28/22 9:56 AM, Jörn-Thorben Hinz wrote: > > > Hi, > > > > > > after compiling a skeleton-using program with -pedantic once and > > > stumbling across a tiniest incorrectness in skeletons with it[1], > > > I was > > > debating whether it makes sense to suppress warnings from > > > skeleton > > > headers. > > > > > > Happy about comments about this. This change might be too > > > suppressive > > > towards warnings and maybe ignoring only -Woverlength-strings > > > directly > > > in OBJ_NAME__elf_bytes() be a better idea. Or keep all warnings > > > from > > > skeletons available as-is to have them more visible in and around > > > bpftool’s development. > > > > This is my 2cents. As you mentioned, skeleton file are per program > > and not in system header file directory. I would like not to mark > > these header files as system files. Since different program will > > generate different skeleton headers, suppressing warnings > > will prevent from catching potential issues in certain cases. > > > > Also, since the warning is triggered by extra user flags like - > > pedantic > > when building bpftool, user can also add -Wno-overlength-strings > > in the extra user flags. > > I agree with Yonghong, I don't think it's a good idea to mark the > whole > file as a system header. I would maybe consider the other solution > where > we can disable the warning locally in the skeleton, just around > OBJ_NAME__elf_bytes() as you suggested. That was my first attempt. But, after a second look, -Wsign-conversion, -Wcast-qual, and -Wreserved-id-macro would also show a warning for a skeleton header. That’s why I switched to the `GCC system_header`. Ignoring these four warnings for the whole header would also be an alternative. Doing that for all of them only at the place where they are triggered might become less pretty. Please also see my longer answer to Yonghong. > Although I suppose we'd need > several pragmas if we want to silence it for GCC and clang, for > example? > It looks like your patch was only addressing GCC? It is only explicitly mentioned for `GCC diagnostic …` in [1], but clang seems to support the `GCC system_header` for compatibility, too. So both are covered by using GCC’s pragmas here. [1] https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas (also the following subsection) > > Thanks for the contribution, Thanks for your reply! > Quentin
