OpenHFT / Chronicle-Values

http://chronicle.software
Other
104 stars 37 forks source link

Difference in behaviour between heap and native types - CharSequence properties #9

Closed JerrySheaB4 closed 8 years ago

JerrySheaB4 commented 8 years ago

The generated code for heap classes stores a reference to a CharSequence in a field and allows this to be changed whereas the generated flyweight copies chars in and out in the setter & getter. Generated code for the heap class:

  private CharSequence __fieldsymbol;
  @Override
  public CharSequence getSymbol() {
    return __fieldsymbol;
  }
  @Override
  public void setSymbol(CharSequence _symbol) {
    this.__fieldsymbol = _symbol;
  }

Surely these should behave the same? Seems like heap class should change to behave like the flyweight. Assuming you agree, I'm happy to fork and fix.

Cheers

Please see below failing test

package com.bigpaws;

import net.openhft.chronicle.bytes.Byteable;
import net.openhft.chronicle.bytes.BytesStore;
import net.openhft.chronicle.values.MaxUtf8Length;
import net.openhft.chronicle.values.NotNull;
import net.openhft.chronicle.values.Values;
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;

public class HeapVsNativeTest {
    public static final String SYMBOL = "symbol";

    @Test
    public void heap() {
        Entity entity = Values.newHeapInstance(Entity.class);
        check(entity);
    }

    @Test
    public void nativeRef() {
        Entity entity = Values.newNativeReference(Entity.class);
        byte[] bytes = new byte[7];
        BytesStore bs = BytesStore.wrap(bytes);
        Byteable byteable = (Byteable) entity;
        byteable.bytesStore(bs, 0, bytes.length);
        check(entity);
    }

    private void check(Entity entity) {
        entity.setSymbol(SYMBOL);
        assertTrue(SYMBOL.contentEquals(entity.getSymbol()));
        assertNotEquals(SYMBOL, entity.getSymbol());
    }

    public static interface Entity {
        CharSequence getSymbol();
        void setSymbol(@NotNull @MaxUtf8Length(6) CharSequence symbol);
    }
}
leventov commented 8 years ago

Yes, heap implementation should copy contents of charSeqs/stringBuilders, passed to set() method. I think it shouldn't copy contents of Strings, because they are immutable. Yes, the current implementation never copies. Looks like this is simply an omission, not wrong design/intention. I'm working on it.