[PATCH] D47843: Introducing single for loop into clang_proto_fuzzer

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: tools/clang-fuzzer/CMakeLists.txt:28
   protobuf_generate_cpp(PROTO_SRCS PROTO_HDRS cxx_proto.proto)
+  protobuf_generate_cpp(PROTO_SRCS PROTO_HDRS cxx_loop_proto.proto)
   set(LLVM_OPTIONAL_SOURCES ${LLVM_OPTIONAL_SOURCES} ${PROTO_SRCS})
----------------
I think it makes sense to use separate SRCS and HDRS variables for cxx_loop_proto.proto.  Otherwise each proto-fuzzer will be compiled with //both// protobufs even though each only uses one.


================
Comment at: tools/clang-fuzzer/CMakeLists.txt:58
   add_clang_subdirectory(fuzzer-initialize)
 
   # Build the protobuf fuzzer
----------------
Why is this here twice?


================
Comment at: tools/clang-fuzzer/CMakeLists.txt:90
+    clangLoopProtoToCXX
+    )
 endif()
----------------
Maybe you can cut down on some LOC here by creating a `COMMON_PROTO_FUZZ_LIBRARIES` variable with the dependencies that overlap between the proto-fuzzers.


================
Comment at: tools/clang-fuzzer/proto-to-cxx/loop_proto_to_cxx.h:22
+std::string LoopProtoToCxx(const uint8_t *data, size_t size);
+}
----------------
morehouse wrote:
> Instead of making a whole new header for this, can we simply add  `LoopProtoToCxx()` and `LoopFunctionToString()` to proto_to_cxx.h?  Then implement them in `loop_proto_to_cxx.cpp`?
Can we remove this file completely?


Repository:
  rC Clang

https://reviews.llvm.org/D47843