[llvm-commits] [llvm] r153221 - in /llvm/trunk: include/llvm/ExecutionEngine/ lib/ExecutionEngine/MCJIT/ lib/ExecutionEngine/RuntimeDyld/ test/ test/ExecutionEngine/MCJIT/ tools/llvm-rtdyld/

[Chandler Carruth]

Some random style and coding convention comments that jumped out at me
while skimming this.

There are probably more similar things that would be worth cleaning up in
the rest of the patch.

On Wed, Mar 21, 2012 at 2:06 PM, Danil Malyshev
<dmalyshev at accesssoftek.com>wrote:

> +  // FIXME: ObjectFile don't modify MemoryBuffer.
> +  //        It should use const MemoryBuffer as parameter.
> +  ObjectFile *obj = ObjectFile::
> +
>  createObjectFile(const_cast<MemoryBuffer*>(InputBuffer));
>

This indentation is pretty hard to read and track. I would really prefer to
see line breaks between operators/operands and after '('s rather than
inside of qualified names.

Many LLVM files write:

LongTypeName *foo
  = reallyBigInitializerFactoryFunction(x, y, z);


> +
> +  Arch = (Triple::ArchType)obj->getArch();
> +
> +  LocalSymbolMap LocalSymbols;     // Functions and data symbols from the
> +                                   // object file.
> +  ObjSectionToIDMap LocalSections; // Used sections from the object file
> +
> +  error_code err;
> +
> +
> +  // Parse symbols
> +  DEBUG(dbgs() << "Parse symbols:\n");
> +  for (symbol_iterator it = obj->begin_symbols(), itEnd =
> obj->end_symbols();
>

Extremely strong convention is to write iterator names as:

for (foo_iterator I = foo->begin(), E = foo->end()

The 'it' and 'itEnd' don't even follow the conventions for local variables.

+       it != itEnd; it.increment(err)) {
> +    if (err) break;
>

Please just put this in the loop condition: 'i != E && !err'

+    object::SymbolRef::Type SymType;
> +    StringRef Name;
> +    if ((bool)(err = it->getType(SymType))) break;
>

Please follow the local variable naming convention even here, and just use
a condition variable for this -- it's how error_code was meant to be used

if (error_code Error = it->getType(SymType))
  report_fatal_error(Error.message());

Reporting the error directly from the if I think is easier on the reader,
because now there is no control flow to understand. (It's also a touch more
efficient in some cases...)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120322/74195f70/attachment.html>