[PATCH] D40837: [FuzzMutate] Allow only sized pointers for the GEP instruction

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: unittests/FuzzMutate/OperationsTest.cpp:16-19
 #include "llvm/IR/Verifier.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include "llvm/Support/SourceMgr.h"
----------------
Please keep these sorted. clang-format can do that for you.


================
Comment at: unittests/FuzzMutate/OperationsTest.cpp:57-59
+namespace {
+std::unique_ptr<Module> parseAssembly(
+    const char *Assembly, LLVMContext &Context) {
----------------
I don't know gtest all that well, but is putting all of this in an anonymous namespace the right thing to do here? Why not just make parseAssembly static?


================
Comment at: unittests/FuzzMutate/OperationsTest.cpp:179-180
   Module M("M", Ctx);
   Function *F = Function::Create(FunctionType::get(Type::getVoidTy(Ctx), {},
-                                                   /*isVarArg=*/false),
+                                     /*isVarArg=*/false),
                                  GlobalValue::ExternalLinkage, "f", &M);
----------------
Apparently phabricator only shows the diff here in the email and not the we interface, but what's with the arbitrary whitespace changes  on these Function::Create calls? According to clang-format, the old way is correct.


================
Comment at: unittests/FuzzMutate/OperationsTest.cpp:294-298
+  // Don't match %v
+  ASSERT_FALSE(Descr.SourcePreds[0].matches({}, &*BB.begin()));
+
+  // Match %a
+  ASSERT_TRUE(Descr.SourcePreds[0].matches({}, &*std::next(BB.begin())));
----------------
I feel like these would be easier to understand / wouldn't need the comments if we create the instructions programmatically like in the other tests in this file rather than parsing it.


https://reviews.llvm.org/D40837