[PATCH] D58704: Initial (incomplete) implementation of JITLink - A replacement for RuntimeDyld.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp:282-288
+                auto &LHSP = LHS->getSection();
+                auto &RHSP = RHS->getSection();
+                if (LHSP.getSectionOrdinal() < RHSP.getSectionOrdinal())
+                  return true;
+                if (LHSP.getSectionOrdinal() > RHSP.getSectionOrdinal())
+                  return false;
+                return LHS->getOrdinal() < RHS->getOrdinal();
----------------
lhames wrote:
> dblaikie wrote:
> > might be easier written as:
> > 
> >   return std::tie(LHSP.getSectionOrdinal(), LHS->getOrdinal()) < std::tie(RHSP.getSectionOrdinal(), RHS->getOrdinal());
> That's a neat trick, but once formatted for 80 columns I am not sure it's any more readable.
FWIW, I find the tie version easier to read because I don't have to think about the interesting true/false/less than/greater than cases in the custom version you have here. I can't eyeball exactly what this ordering is.


================
Comment at: unittests/ExecutionEngine/JITLink/JITLinkTestCommon.h:97-99
+      return support::endian::read<T, 1>(DA->getContent().data() + Offset,
+                                         G.getEndianness());
+    else
----------------
lhames wrote:
> dblaikie wrote:
> > else-after-return
> > 
> > Ah, I see, scoping because of "DA" - I'd probably move DA out of the condition and early return for the error case instead:
> > 
> >   auto DA = G.find...;
> >   if (!DA)
> >     return DA.takeError();
> >   return support::endian::read<>(...);
> > 
> > Reduces indentation of the main path
> Interesting. I tend to prefer the if/else framing because it feels like the "success/failure" state on each path is more obvious. I only switch to early outs for Expected values if I think the indentation will make things really unreadable.
*nod*

For myself, I prefer the "conditionalize the failure/error handling" as a consistent strategy (since it's more necessary in longer functions (where indenting every successful branch starts to get a bit arrow-y ( https://blog.codinghorror.com/flattening-arrow-code/ )) , but I like to adopt a format that's consistent across short and longer code (so it doesn't need to be restructured if there ends up being a second error path in the code)

Yeah, I wish there was slightly tidier syntax for it - for sure (the convenience of variable declared in if scopes is kinda nice - makes it very clear that the if/else block only occurs when the entity is true/false - rather than having to parse some (even just slightly, with a single !) non-trivial condition to check)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58704/new/

https://reviews.llvm.org/D58704