[vtk] Unify include spelling, add comments, fix deps #20024
Conversation
jwnimmer-tri
added
priority: low
status: single reviewer ok
|
+@rpoyner-tri for both reviews, if you please. I'll propose this doesn't necessarily warrant a full cross-check, but rather more like a spot check + endorsement of the approach. \CC @SeanCurtis-TRI for awareness. |
VTK splits its code into libraries, but doesn't use subdir include paths to reflect the which library has which header. This makes it difficult to properly update BUILD files as the include statements evolve. For clarity, we now mark all VTK includes with the library they come from. We can also add some missing deps now as a result.
jwnimmer-tri
force-pushed
the
vtk-include-comments
branch
from
c6c2122
to
495a1bf
Compare
There was a problem hiding this comment.
/me glances a "What would Lakos do?" rubber bracelet
Sigh. Is there any tool support for maintaining the annotations?
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
There was a problem hiding this comment.
Maybe it would be good enough to have write-up somewhere to guide maintainers for this idiom.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
It's difficult for me to put myself in those shoes and guess what they might need. Here's some brain dumping: For a VTK header we already use somewhere, the easy answer is to cargo cult from an existing Drake If a developer wants to use a particular VTK header for the first time inside Drake, the trick is to find the header in the VTK source tree (e.g., https://github.com/Kitware/VTK/blob/master/IO/Image/vtkImageExport.h) and so As a last resort, I could add that to the commit message. I wonder where else someone would look. Maybe |
There was a problem hiding this comment.
Yup, I grant that knowing where to write is a hard problem. I'd settle for commit message now, and we see what needs arise later.
Reviewed 27 of 27 files at r1, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignee rpoyner-tri(platform)
Done. |
Towards #19945 and #16502.
This change is