[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

Steven Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 12 13:23:57 PDT 2022


steven_wu added inline comments.


================
Comment at: cmake/Modules/FindGRPC.cmake:111
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
----------------
sammccall wrote:
> steven_wu wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > changing from function to macro means all the local variables here are done in the parent scope, which is hard to reason about.
> > > > 
> > > > I think the only reason you're doing that here is to return the filename in the `ProtoSource` variable, in which case `set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE)` should do the job from a function.
> > > nit: the plural is in the wrong place: rather `generate_proto_sources`?
> > > this generates the multiple sources for a single proto, rather than the other way around.
> > > 
> > > (I'm not sure why the original was plural, but this makes it somehow more confusing)
> > The reason why it is a macro is I don't know any elegant way to pass all the DEPEND down to `add_*_library`.
> > The reason why it is a macro is I don't know any elegant way to pass all the DEPEND down to add_*_library.
> 
> ... oh, yikes, we're passing through by pretending they're `${ProtoFile}`, I didn't see that :-\
> 
> I guess the easiest option is just to change that name to something like `ProtoArgs` in the wrapping function, and add a bit of documentation.
> I think using CMAKE_PARSE_ARGUMENTS in the wrapping function is an option though, and then pass the unparsed arguments through?
Ah, I just learnt the unparsed_argument option. Then it is not too bad. Fixing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135712/new/

https://reviews.llvm.org/D135712



More information about the cfe-commits mailing list