New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate __hash__ on python as well #76

Merged
merged 4 commits into from Sep 18, 2016

Conversation

Projects
None yet
3 participants
@kanghyojun
Member

kanghyojun commented Sep 16, 2016

implement #75

@dahlia

제 생각에는 해시 충돌에 관한 케이스에 대해서는 확률이 충분히 낮기 때문에 실제에서 큰 문제가 되지는 않으리라고 봅니다. 하지만 CI 등에서 반복적으로 빌드를 하다보면 그런 일이 일어날 수도 있습니다.

@@ -200,6 +200,12 @@ compileUnionTag source parentname typename' fields = do
slots = if length tagNames == 1
then [qq|'{head tagNames}'|] `T.snoc` ','
else toIndentedCodes (\n -> [qq|'{n}'|]) tagNames ",\n "
hashTuple = if null tagNames
then "self.__nirum_tag__"
else [qq|tuple([self.__class__, {attributes}])|] :: T.Text

This comment has been minimized.

@dahlia

dahlia Sep 16, 2016

Member

해시할 때 타입 정보 안 넣어도 됩니다. 파이썬 hash()가 원래 같은 타입 안에서만 충돌이 안 나면 (확률적으로 충분히 낮게 나면) 되거든요. 내부적으로 hash()를 사용하는 dict/set 등이 버킷 나눌 때 hash(v)만 가지고 나누는 게 아니라 (type(v), hash(v)) 쌍을 기준으로 나눕니다. 따라서 저희가 이걸 할 필요가 없어요.

그리고 (a, b, c) 같은 식으로 써도 될텐데 tuple([a, b, c]) 식으로 출력되게 만드셨네요.

This comment has been minimized.

@kanghyojun

kanghyojun Sep 17, 2016

Member

tuple()로 한거는 사실 위에 코드에도있지만 argument가 한개일때 따로 trailing comma를 넣는 처리를 하기싫어서 저렇게 해놓긴했습니다.

tuple([a]) 이건괜찮지만, (a)이건사실 tuple로 평가되질 않는 문제가 ㅜㅜ..

@@ -325,7 +334,7 @@ class $className:
self.value == other.value)

def __hash__(self) -> int:
return hash(self.value)
return hash((self.__class__, self.value))

This comment has been minimized.

@dahlia

dahlia Sep 16, 2016

Member

여기도 마찬가지입니다. 타입은 안 넣어도 됩니다.

@@ -360,6 +372,7 @@ class $className(enum.Enum):
@classmethod
def __nirum_deserialize__(cls: type, value: str) -> '{className}':
return cls(value.replace('-', '_')) # FIXME: validate input

This comment has been minimized.

@dahlia

dahlia Sep 16, 2016

Member

이 줄바꿈은 잘못 섞여들어간 건가요, 아니면 일부러 한칸 더 넣은 건가요?

@@ -381,6 +394,9 @@ compileTypeDeclaration src TypeDeclaration { typename = typename'
toNamePair
[name | Field name _ _ <- toList fields]
",\n "
hashTuple = [qq|(self.__class__, {attributes})|] :: T.Text

This comment has been minimized.

@dahlia

dahlia Sep 16, 2016

Member

여기도 마찬가지입니다. 타입은 안 넣어도 됩니다.

tT decl [q|hash(Point(left=3, top=14)) ==
hash(Point(left=3, top=14))|]
tT decl [q|hash(Point(left=3, top=14)) !=
hash(Point(left=1, top=592))|]

This comment has been minimized.

@dahlia

dahlia Sep 16, 2016

Member

사실 이 테스트는 항상 성공한다고 보장할 수가 없습니다. 해시 함수의 특성을 생각해봅시다:

해시 함수의 가장 기본적인 성질은 두 해시 값이 다르다면 원래의 데이터도 다르다는 것이다. 이 특징은 해시 함수가 결정적이기 때문이다. 반대로 해시 함수는 단사 함수가 아니다. 같은 해시 값을 갖더라도 원래의 입력값이 같다는 것을 시사하지만 보장해주지는 않는다. 원래 입력의 한 비트만 바뀌더라도 해시 함수의 성질로 인해 해시 값은 크게 달라진다.

hash(x)hash(y)의 결과가 다르다면 xy도 다르다는 뜻입니다. 하지만 hash(a)hash(b)의 결과가 같다고 해서 ab도 서로 같으리라 보장할 수는 없습니다. (인용한 것처럼 시사할 수는 있습니다.) 반대로 얘기하면 ab가 다른 값이지만 우연히 hash(a)hash(b)가 같은 결과를 내는 해시 충돌을 만날 수도 있습니다. 따라서 위 테스트에서 아래쪽 단언문은 항상 성공을 보장하진 않습니다.

This comment has been minimized.

@kanghyojun

kanghyojun Sep 17, 2016

Member

리뷰 감사합니다

tT decl [q|hash(WesternName(first_name='foo', middle_name='bar',
last_name='baz')) !=
hash(WesternName(first_name='hello', middle_name='bar',
last_name='baz'))|]

This comment has been minimized.

@dahlia

dahlia Sep 16, 2016

Member

여기도 마찬가지입니다. 해시 충돌이 있을 수 있으므로 이 단언문은 항상 성공하지는 않습니다.

tT decl [q|hash(WesternName(first_name='foo', middle_name='bar',
last_name='baz')) !=
hash(WesternName2(first_name='foo', middle_name='bar',
last_name='baz'))|]

This comment has been minimized.

@dahlia

dahlia Sep 16, 2016

Member

여기도 마찬가지입니다. 해시 충돌이 있을 수 있으므로 이 단언문은 항상 성공하지는 않습니다.

kanghyojun added a commit to kanghyojun/nirum-1 that referenced this pull request Sep 17, 2016

@kanghyojun kanghyojun force-pushed the kanghyojun:hash-gen branch from f06b5a2 to 700ab63 Sep 18, 2016

@dahlia

dahlia approved these changes Sep 18, 2016

@dahlia dahlia merged commit 6bbcdd8 into nirum-lang:master Sep 18, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment