sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
thanks!
================
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")
----------------
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.
================
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:
> 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)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135712/new/
https://reviews.llvm.org/D135712
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
