Annotate impl/codegen with IWYU pragmas #27252
Conversation
ctiller
changed the title
include/grpc/impl/codegen/atm.h
Outdated
| @@ -19,6 +19,8 @@ | |||
| #ifndef GRPC_IMPL_CODEGEN_ATM_H | |||
| #define GRPC_IMPL_CODEGEN_ATM_H | |||
|
|
|||
| // IWYU pragma: private, include <grpc/impl/codegen/atm.h> | |||
There was a problem hiding this comment.
I'm not sure what this accomplishes, the specified alternative file is the <...> way of naming itself. Is this so that IWYU suggests our preferred system-header-style include format?
There was a problem hiding this comment.
Nah it's got the wrong path... good catch!
|
Do I understand correctly that the goal is to replace most (maybe all?) |
|
Within codegen we want to include from codegen, from everywhere else it's
probably better to include from support.
codegen itself is for protobuf generated code to depend on, we want to
guide real code to include from non-codegen paths, *especially* customer
code.
If customer code depends on codegen, then some internal tools will point
users to depend on grpc_codegen or grpc++_codegen_base to pass layering
checks - which is minimally weird, but often plain wrong: grpc or grpc++
*need* to be depended on to pull in the actual RPC engine, and omitting
such is an error.
…On Tue, Sep 7, 2021 at 2:43 PM AJ Heller ***@***.***> wrote:
Do I understand correctly that the goal is to replace most (maybe all?) #include
".*/codegen/.*" instances with their support path equivalents? For
example, we have ~120 places that use #include
<grpc/impl/codegen/port_platform.h>, but ~900 places that use the
<grpc/support/port_platform.h> version. Is there anywhere that internal
code *should* be including from the codegen folder directly, maybe within
the codegen folder itself?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#27252 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACNG45IM45OQNAZ7T3BH23LUA2BQ7ANCNFSM5DPQWBLA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
There was a problem hiding this comment.
Maybe I'm missing something in the pragma script, but for example, I don't see what would prevent IWYU from suggesting that the includes in include/grpc/impl/codegen/sync.h need to change to use <grpc/support/sync_generic.h> instead of the current include path <grpc/impl/codegen/sync_generic.h>.
tools/distrib/add-iwyu.py
Outdated
| def fix_tree(tree): | ||
| """Fix one include tree""" | ||
| # Map of filename --> paths including that filename | ||
| reverse_map = {} |
|
The sync_generic example is interesting... and yeah I think it needs handling too. I don't think this is the end state for these annotations, rather I expect to be iterating on them for a while - as we can grab real code and see what it recommends. I'm also trying to bound time here a little bit - this isn't high high priority, but would help some folks out. |
|
ok nerd sniped... added some hacks to the scripts to get better behavior with atm, sync |
…e406 Automated fix for refs/heads/iwyu
| @@ -18,7 +18,7 @@ | |||
| #ifndef GRPCPP_IMPL_CODEGEN_SYNC_STREAM_H | |||
| #define GRPCPP_IMPL_CODEGEN_SYNC_STREAM_H | |||
|
|
|||
| // IWYU pragma: private, include <grpcpp/support/sync_stream.h> | |||
| // IWYU pragma: private, include <grpc/support/sync.h> | |||
There was a problem hiding this comment.
This needs an exception, sync_stream is not a sync alternative.
There was a problem hiding this comment.
I think done before this review?
There was a problem hiding this comment.
Ah, you're right, I wasn't looking at the complete diff.
My only hold-up is whether IWYU would be useful if it does the wrong thing on our own codebase, and needs manual intervention every time we run it (adding back |
|
We've (AFAICT) never run IWYU on our codebase - I tried early on - the OSS version is hard to set up and marked as alpha quality software -- and so the likelihood that we embrace it soon is low. If we do so, often it ends up being a substantial project the first time through adding pragma's to necessary headers to make sure that those inclusions are not touched. In the meantime, we've got a I can add a comment somewhere, but I don't know where would be useful. |
|
(also note that we plan to eliminate |
copybara-service
bot
added
the
imported
* Add a tool to annotate impl/codegen with IWYU pragmas * xx * oops * fmt * x * fix wrong direction bug * use defaultdict * better annotations * better annotations * Automated change: Fix sanity tests Co-authored-by: ctiller <ctiller@users.noreply.github.com>
Help users of this tool land on the right headers.
Add a tool to keep this up to date.
@nicolasnoble