[Unittest] Clean up formatting, NFC

[JosephTremoulet]

Summary

Use an early return to reduce indentation.
Remove unused local.

[lhames]

LGTM. Thanks Joseph.

[dblaikie]

unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp

327

I think I mentioned this during post-commit review, perhaps.

It's probably possible to just write "{}" for the first parameter - skipping both the local variable and the inline construction. (though, FWIW, I'd probably take the inline construction over the named variable if those were the choices - but I can see the alternative point of view/wouldn't say it's clear cut either way)

[lhames]

unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp

327

addModuleSet is a template method - there's no way for {}'s type to be deduced. Inline construction seems like an option though.

[JosephTremoulet]

unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp

327

Yeah, sorry, I should have explained in a comment. As Lang said, it's a template method and the first argument's type is a template parameter, so {} defeats inference. That said, I could explicitly specify the template arguments rather than relying on deduction and then use {} -- would that be preferable? I just assumed this was preferable because explicitly specifying the parameters would still have the inline type expression, which I thought was what you were objecting to, though clearly I've misunderstood your intent if the inline constructor is now preferable...

[JosephTremoulet]

switch back to inline ctor, as that seems to be the consensus

[dblaikie]

unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp

282

Also, it might be simpler to a separate function, attributed with LLVM_UNUSED (so no warnings fire on it) rather than the volatile approach.

(also, I worry about API testing like this - if we have a formal layer concept we want to implement - we should be able to (& we should) check that independently of any particular implementation (& it should check the semantics as well - as we are testing above, presumably). To keep the tests isolated, to avoid slipping API boundaries, narrow tests, etc)

[JosephTremoulet]

unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp

282

it might be simpler to a separate function, attributed with LLVM_UNUSED (so no warnings fire on it) rather than the volatile approach

OK, I can do that. The comment at the definition of LLVM_ATTRIBUTE_UNUSED mentions "Prefer cast-to-void wherever it is sufficient" because it's more portable, so I wonder: would it be better still to wrap this in a lambda and then just cast the closure to void?

(also, I worry about API testing like this - if we have a formal layer concept we want to implement - we should be able to (& we should) check that independently of any particular implementation (& it should check the semantics as well - as we are testing above, presumably). To keep the tests isolated, to avoid slipping API boundaries, narrow tests, etc

Yeah, it felt hokey adding a "just make sure this compiles" test. I went forward with it because it is an improvement over the current state of things and I verified that it would have caught the regression that hit us downstream. I'd be happy to be pointed in the direction of something more proper (e.g. is there some way to capture this as a static_assert over type traits or something?), but I don't see what that would be. You predicated the suggestion on "if we have a formal layer concept", and ISTM the issue here is that we have only an informal layer concept, because the formalism we want is precisely a concept describing object layers, but concepts haven't made it into the language yet. I think that our informal "concept" of an object layer here is "something that has the same signature as the ObjectLinkingLayer", which is why this test conjoins the ObjectTransformLayer being tested specifically to an ObjectLinkingLayer instance, and why there are no other similar tests (we don't, to my knowledge, have any other object layers in-tree).
To make it more concrete, ObjectLinkingLayer and the informal "object layer" concept have a template method with this signature:

template <typename ObjSetT,
          typename MemoryManagerPtrT,
          typename SymbolResolverPtrT>
ObjSetHandleT addObjectSet(ObjSetT Objects,
                           MemoryManagerPtrT MemMgr,
                           SymbolResolverPtrT Resolver)

This test was added with rL258630, which fixed a regression introduced by rL258185 adjusting the signature of the ObjectLinkingLayer (and the general/informal "object layer" "concept"), without updating the ObjectTransformLayer. The signature change actually looks benign, as the change was of the Objects parameter from const ObjSetT &Objects to ObjSetT Objects, but the issue is that the ObjectTransformLayer's attempt to forward Objects then changed to involve a copy, and ObjSetT can be instantiated to a non-copyable type. This was the best way I thought of to write a test that would catch that sort of change, but I'm happy for suggestions on a better way.

Lastly, I'm afraid I don't entirely understand what you mean regarding testing semantics when you say "check that independently of any particular implementation (& it should check the semantics as well - as we are testing above, presumably" -- the object layer concept, as I understand it, is essentially just a signature, so I'm not following what semantics we'd check in a way that's independent of any particular implementation. The ObjectTransformLayer in particular has the semantics that it applies its transformation to objects passed in and forwards everything else to the base layer; yes that is what the code above is testing, but I'm not following how you're suggesting it be restructured.

unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp

TEST(ObjectTransformLayerTest, Main) {
 
   // Verify transform getter (const)
   auto OwnedObj = llvm::make_unique<MockObjectFile>(288);
   const auto &T1C = T1;
   OwnedObj = T1C.getTransform()(std::move(OwnedObj));
   EXPECT_EQ(289, *OwnedObj) << "Expected incrementing transform";
 
   volatile bool RunStaticChecks = false;
-  if (RunStaticChecks) {
-    // Make sure that ObjectTransformLayer implements the object layer concept
-    // correctly by sandwitching one between an ObjectLinkingLayer and an
-    // IRCompileLayer, verifying that it compiles if we have a call to the
-    // IRComileLayer's addModuleSet that should call the transform layer's
-    // addObjectSet, and also calling the other public transform layer methods
-    // directly to make sure the methods they intend to forward to exist on
-    // the ObjectLinkingLayer.
+  if (!RunStaticChecks)
+    return;
+
 
-    // We'll need a concrete MemoryManager class.
-    class NullManager : public llvm::RuntimeDyld::MemoryManager {
-    public:
-      uint8_t *allocateCodeSection(uintptr_t, unsigned, unsigned,
-                                   llvm::StringRef) override {
-        return nullptr;
-      }
-      uint8_t *allocateDataSection(uintptr_t, unsigned, unsigned,
-                                   llvm::StringRef, bool) override {
-        return nullptr;
-      }
-      void registerEHFrames(uint8_t *, uint64_t, size_t) override {}
-      void deregisterEHFrames(uint8_t *, uint64_t, size_t) override {}
-      bool finalizeMemory(std::string *) { return false; }
-    };
+    uint8_t *allocateDataSection(uintptr_t, unsigned, unsigned, llvm::StringRef,
+                                 bool) override {
 
-    // Construct the jit layers.
-    ObjectLinkingLayer<> BaseLayer;
-    auto IdentityTransform = [](
-        std::unique_ptr<llvm::object::OwningBinary<llvm::object::ObjectFile>>
-            Obj) { return std::move(Obj); };
-    ObjectTransformLayer<decltype(BaseLayer), decltype(IdentityTransform)>
-        TransformLayer(BaseLayer, IdentityTransform);
-    auto NullCompiler = [](llvm::Module &) {
-      return llvm::object::OwningBinary<llvm::object::ObjectFile>();
-    };
-    IRCompileLayer<decltype(TransformLayer)> CompileLayer(TransformLayer,
-                                                          NullCompiler);
-    std::vector<llvm::Module *> Modules;
 
-    // Make sure that the calls from IRCompileLayer to ObjectTransformLayer
-    // compile.
-    NullResolver Resolver;
-    NullManager Manager;
-    CompileLayer.addModuleSet(std::vector<llvm::Module *>(), &Manager,
-                              &Resolver);
+  std::vector<llvm::Module *> Modules;
+  CompileLayer.addModuleSet(Modules, &Manager, &Resolver);
 
-    // Make sure that the calls from ObjectTransformLayer to ObjectLinkingLayer
-    // compile.
-    decltype(TransformLayer)::ObjSetHandleT ObjSet;
-    TransformLayer.emitAndFinalize(ObjSet);
-    TransformLayer.findSymbolIn(ObjSet, Name, false);
-    TransformLayer.findSymbol(Name, true);
-    TransformLayer.mapSectionAddress(ObjSet, nullptr, 0);
-    TransformLayer.removeObjectSet(ObjSet);
-  }
 }
-}