Eli Friedman eli.friedman at gmail.com
Tue Jul 20 14:09:34 PDT 2010

On Tue, Jul 20, 2010 at 1:36 PM, Olivier Meurant
<meurant.olivier at gmail.com> wrote:
>> Seems reasonable, but I haven't looked at the code yet. I would
>> suggest trying to split your work up into separate patches which
>> implement incremental pieces of functionality, then submitting them to
>> llvm-commits for review. That is much easier for us to deal with than
>> large monolithic patches out of tree.
> Ok. Sorry for the too big patch. Attached is the first patch adding
> only 2 hooks on TargetMachine and on MCAssembler. Style should be LLVM
> compliant. Apply it with "patch -p0".

+  // Make sure the code model is set.
+  setCodeModelForStatic();

For the JIT, this should be setCodeModelForJIT; it makes a difference
on, for example, x86-64.  I'd also suggest changing the name of the
method appropriately.

-  llvm::OwningPtr<MCObjectWriter> Writer(getBackend().createObjectWriter(OS));
-  if (!Writer)
-    report_fatal_error("unable to create object writer!");
+  MCObjectWriter *Writer = Writer_;
+  if (Writer == 0) {
+      Writer = getBackend().createObjectWriter(OS);
+      if (!Writer)
+          report_fatal_error("unable to create object writer!");
+  }

It would be cleaner if you kept around the OwningPtr, but only
assigned to it in the case where the writer take ownership; that way,
you wouldn't have to mess with explicitly deleting the writer.  Also,
the surrounding code uses two spaces for indentation, not four.

I think it looks fine otherwise.


More information about the llvm-dev mailing list