Review Board

1.0

Lots of changes to LanguageKit / Smalltalk for merging to stable.

Updated 1 year, 9 months ago

David Chisnall Reviewers
EtoileCore
None Etoile trunk (etoile/trunk/Etoile) 
Loads of LanguageKit changes for merging to stable.

Includes class variable and method support and numerous bug fixes.
Passes more of günther's tests than the version currently in stable.

New version of test.st included - everything there works on FreeBSD.
Günther Noack
Posted 1 year, 9 months ago (December 3rd, 2008, 3:57 p.m.)
Most of the ObjC code looks good to me. I had trouble with the parts scratching the surface of LLVM (*.cpp and their header files), I can't really judge on that code. The indenting of new code in the *.cpp files looks wrong in my browser.

The changes to the ObjectMixer look obvious, even without having read and understood the whole source code.

The usage of type strings is unclear to me in some cases. As they don't match my expectations of what @encode() should return, I assume they are from LLVM as well? In one place (LKMethod I think), I saw one of those strings being calculated using sizeof(SEL) and sizeof(id). Maybe a platform independent way of suggesting types via pragmas would be better?

I like how moving the code concerned with method types to LKCompilationUnit cleared up LKMessageSend. :-)


Indenting looks different here than for the previous method declaration.
  1. Original indenting was in LLVM style.  New code is in the Étoilé style.  I'm gradually switching it all over.
Here, indenting is different as well.
indent
Languages/LanguageKit/LKAST.m (Diff revision 1) 
 
 
Assigning self looks weird to me. What about the following solution?

- (LKCompilationUnit*) module
{
  if (ModuleClass == isa)
    return self;
  else
    return [parent module];
}

It should behave the same, even when parent is nil, nil is returned. (Maybe it's a good idea to document whether this is intended, though.)
  1. Parent is only nil when it's a module.
It's not clear to me what these type strings do... :-/
  1. This is old code, but it is ensuring that the variable is an object.
How these types work here is unclear to me.
  1. The type encoding for the method is passed to the code generator as an argument.  This is required so that the back end can perform the required boxing of any non-id (@) types.
LKMessageCascade looks very reasonable and clear to me.
Languages/LanguageKit/LKMethod.m (Diff revision 1) 
 
 
When the method type format is concerned with the size of SEL and id, do type pragmas work on 64 bit processors?
  1. Yes, that's the entire point.  Method type signatures include he size of the stack frame.  The format is:
{return type encoding}{size of type frame}@{sizeof(id)}:{sizeof(SEL)} followed by type, size pairs for all other parameters.
Languages/LanguageKit/LKMethod.m (Diff revision 1) 
 
 
The subclassing makes sense.
Languages/LanguageKit/LKModule.m (Diff revision 1) 
 
 
The pragma things look good to me.

I didn't have a look at +initialize yet, as I assume it came from LKMethod unchanged.

I first had problems understanding the meaning of the typeOverrides variable. As I understand it, not the types themselves are overridden, but the type used for method calls is enforced? Is that right? Maybe another name would be suited better. (Anyway, that's definitely not a show stopper bug for inclusion in stable. ;-))

-typeForMethod: looks clearly correct to me, so does the rest of the class.
  1. typeOverrides is a dictionary containing type encodings for selectors that are specified manually rather than being read from the runtime.
Languages/LanguageKit/LKSubclass.m (Diff revision 1) 
 
 
Slight code duplication with the ivarNames, ivarTypes, ivarOffsets code above. Maybe applying the "extract method" refactoring makes sense here.
  1. Could make sense, although in future this code is likely to change when I add support to a runtime with proper class variables, and factoring it out might make it even more complicated, considering that class vars don't have offsets (at the moment...).
Languages/LanguageKit/LKSubclass.m (Diff revision 1) 
 
 
Copy & paste error: cvarOffsets is not used.
  1. Well spotted.
What does this do?
  1. Debugging code, left in by mistake.
Languages/SmalltalkKit/smalltalk.y (Diff revision 1) 
 
 
The cascaded messages look good to me.
Ship it!
Nicolas Roard
Posted 1 year, 9 months ago (December 3rd, 2008, 5:41 p.m.)
Overall LGTM, you should fix the indentation though, and there's quite possibly some asserts that wouldn't be bad to add. Submit after fixes !
  1. I'll add that this is only a quick review of the code, but not of the logic of the code -- I don't know enough about LK+LLVM for that.
Fix the indentation
fix the indentation
fix the indentation
fix the indentation
what happened if *var == NULL ?
idem
fix indentation
indentation
S cannot == NULL ? what happened if GenerateMessageSend fails ?
indentation ?
add assert(CGM) -- or are you sure there is no way CGM or getRuntime() can fail ?
assert(CGM);
what if Runtime == NULL...
InitialiseBuilder can be returned with its value as defined by its default constructor. It's probably ok, but is it what you want, or are you doing some furter initialisation somewhere else (just checking).
fix indentation
fix indentation for those two methods
fix indentation
sizeof(long) * 8 ?.. no magic numbers please.
no magic numbers
add an assert(Runtime) at the beginning
assert(Runtime);
not keen on magic values (yes, even this)
getCurrentScope() can never fails?
Languages/LanguageKit/LKMethod.m (Diff revision 1) 
 
 
Fix indentation
Languages/LanguageKit/LKMethod.m (Diff revision 1) 
 
 
Fix indentation
Languages/LanguageKit/LKModule.h (Diff revision 1) 
 
 
be consistent here -- NSMutableDictionary * typeOverrides; ?
Languages/LanguageKit/LKModule.m (Diff revision 1) 
 
 
fix indentation
Languages/LanguageKit/LKModule.m (Diff revision 1) 
 
 
fix indentation
those chained pointers can never fail ?