sammccall 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")
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135712/new/
https://reviews.llvm.org/D135712
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
