-
Notifications
You must be signed in to change notification settings - Fork 226
Annotate facade header with IWYU export annotation #1427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Without that annotation, tools such as `clang-tidy` or the `clangd` language server (as well as many other tools) will complain about headers not directly providing a symbol if users include `geometry.hpp`; With this annotation, they know. Documentation IWYU https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-begin_exportsend_exports Documentation llvm include cleaner/clang-tidy/clangd https://clangd.llvm.org/design/include-cleaner#iwyu-pragmas Signed-off-by: Henner Zeller <h.zeller@acm.org>
hi Henner, thanks for your PR. Given the fact that it is a Do you annotate all, or more, Boost Libraries? |
I have no strong opinion on this, so I approve. Since in the commit itself there is not much to review, some general observations:
grep "IWYU" boost/libs -n -r
boost/libs/parser/.clang-format:46:CommentPragmas: '^ IWYU pragma:'
boost/libs/locale/.clang-format:60:CommentPragmas: '^ IWYU pragma:'
boost/libs/mysql/.clang-format:88:CommentPragmas: '(^ IWYU pragma:)|(^\\copydoc )|(^ ?\\n)'
boost/libs/nowide/.clang-format:53:CommentPragmas: '^ IWYU pragma:'
boost/libs/histogram/.clang-format:39:CommentPragmas: '^ IWYU pragma:'
boost/libs/redis/.clang-format:84:CommentPragmas: '(^ IWYU pragma:)|(^\\copydoc )|(^ ?\\n)'
boost/libs/yap/.clang-format:46:CommentPragmas: '^ IWYU pragma:'
|
adding a pragma to |
Thank you for clarifying. If I test with the tool, e.g. #if defined(LEVEL0)
#include <boost/geometry.hpp>
#elif defined(LEVEL1)
#include <boost/geometry/geometry.hpp>
#elif defined(LEVEL2)
#include <boost/geometry/geometries/geometries.hpp>
#else
#include <boost/geometry/geometries/box.hpp>
#include <boost/geometry/geometries/point_xy.hpp>
#endif
using point = boost::geometry::model::d2::point_xy<double>;
using box = boost::geometry::model::box<point>;
int main() {
box a;
return 0;
} That gets me (where ./geometry is your branch) include-what-you-use main.cpp -I./geometry/include -I../boost/ -DLEVEL0 # case 1: complains
include-what-you-use main.cpp -I./geometry/include -I../boost/ -DLEVEL1 # case 2: your include, still complains, on your branch
include-what-you-use main.cpp -I./geometry/include -I../boost/ -DLEVEL2 # case 3: deeper include, still complains
include-what-you-use main.cpp -I./geometry/include -I../boost/ # case 4: no complaint (actually including what is used) So this PR, will suppress a warning from a tool that seems arguably valid (including all of boost geometry is usually including more than what is used, there are specific headers for using specific interfaces) but only if boost geometry is included in way different from what is documented and what most people are probably doing (<boost/geometry/geometry.hpp> instead of <boost/geometry.hpp>) and then not in all cases, only if at least one class or method is used that is declared in a header directly included by boost/geometry.hpp. Is this correct and could any of this be improved? Would we also want this pragma on e.g. boost/geometry/geometries/geometries.hpp and similar headers like these (in particular the facade headers that are recommended for use by the current boost documentation)? If I add the pragma to the geometries.hpp header, it silences the warning for case 3, still not for case 2 though. |
It looks like there is no perfect solution as IWYU only follows the exports pragmas one level deep (I am not using the include-what-you-use but the Maybe the private pragma can help ? https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-private ... i have not explored that |
@@ -20,6 +20,8 @@ | |||
#ifndef BOOST_GEOMETRY_GEOMETRY_HPP | |||
#define BOOST_GEOMETRY_GEOMETRY_HPP | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good to add a comment (one or two sentences) giving the background,
like you did in the PR (maybe more concise). Otherwise this will raise questions after a year or two (by possibly new developers).
Without that annotation, tools such as
clang-tidy
or theclangd
language server (as well as many other tools) will complain about headers not directly providing a symbol if users includegeometry.hpp
; With this annotation, they know.Documentation IWYU
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-begin_exportsend_exports
Documentation llvm include cleaner/clang-tidy/clangd https://clangd.llvm.org/design/include-cleaner#iwyu-pragmas