Skip to content
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

[Closes #471] Use where bound to const check at compilation time #592

Closed
wants to merge 4 commits into from

Conversation

travis1829
Copy link
Collaborator

@travis1829 travis1829 commented Feb 4, 2022

Partially resolves #471
#471 (comment)

PR#489와 비슷하게 여전히 error message를 띄우는 건 안되지만, 그래도 이전보다는 훨씬 깔끔한 방법을 사용해봤습니다.

pub fn as_uninit_mut<T>(&mut self) -> &mut MaybeUninit<T>
where
	[u8; PGSIZE - mem::size_of::<T>()]:,                // We need mem::size_of::<T>() <= PGSIZE
        [u8; PGSIZE % mem::align_of::<T>() + usize::MAX]:   // We need PGSIZE % mem::align_of::<T> == 0
{
	//...
}

Rust blog 등을 확인해보면, 아직은 위 예처럼 array length부분 등을 이용해 검사를 해보기를 권고하는 것 같습니다.
(추후에 훨찐 직관적인 syntax를 제공하겠다고는 했으나, 아직 안 나온 것 같습니다.)

@travis1829
Copy link
Collaborator Author

또는 다음과 같이 자동으로 impl되는 trait을 추가해서 해결할수도 있습니다.
ex)

pub fn as_uninit_mut<T: FromPage>(&mut self) -> &mut MaybeUninit<T> {
	//...
}

/// This trait is auto-implemented for types that suffice the following.
/// * mem::size_of::<T>() <= PGSIZE
/// * PGSIZE % mem::align_of::<T>() == 0
pub trait FromPage {}
impl<T> FromPage for T
where
	[u8; PGSIZE - mem::size_of::<T>()]:,                // We need mem::size_of::<T>() <= PGSIZE
        [u8; PGSIZE % mem::align_of::<T>() + usize::MAX]:   // We need PGSIZE % mem::align_of::<T> == 0
{}


pub fn as_uninit_mut<T>(&mut self) -> &mut MaybeUninit<T>
where
[u8; PGSIZE - mem::size_of::<T>()]: , /* We need mem::size_of::<T>() <= PGSIZE */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/* */ 대신 // 로 해주시겠어요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음에는 //로 했었는데, cargo fmt가 강제로 80번줄만 /* */로 바꾸길래 그냥 /* */로 통일했습니다.

pub fn as_uninit_mut<T>(&mut self) -> &mut MaybeUninit<T>
where
[u8; PGSIZE - mem::size_of::<T>()]: , /* We need mem::size_of::<T>() <= PGSIZE */
[u8; PGSIZE % mem::align_of::<T>() + usize::MAX]: , /* We need PGSIZE % mem::align_of::<T> == 0 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

약간 anti-pattern이 아닌가 생각하긴 합니다 ㅎㅎ 그냥 원래 코드에 비해 어떤 장점이 있을까요?

Copy link
Collaborator Author

@travis1829 travis1829 Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • assert을 compile time check으로 대체한 것 뿐이므로, runtime 비용이 없어진다는 장점 정도만 있는 것 같습니다. (다만, 이걸 참고하면 이미 runtime 비용이 없던 상황일 수도 있습니다.)

일단은 위 expression은 다른 곳들(예시, Rust blog 예)에서도 사용하는 방법이긴 하지만, 자주 쓰이는 것 같지는 않고 제 생각에도 비직관적인 것 같습니다. 좀 걸리겠지만 차라리 Rust blog에서 약속한 대로 더 직관적인 syntax를 정식으로 지원할 때까지 기다리는게 나을 수 있을 것 같습니다.

Copy link
Collaborator Author

@travis1829 travis1829 Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가 : 61a7a43 에서 아무 의미 없는 u8()로 변경했습니다.

@travis1829
Copy link
Collaborator Author

아직까지는 const_evaluatable_checkedgeneric_const_exprs feature가 unstable하므로 버그 등을 일으킬 수 있고 syntax가 깔끔하지 못한 것 같으므로, 이 문제는 추후에 rustc version을 올린 후 revisit하는게 좋을 것 같습니다. 일단은 close합니다.

@travis1829 travis1829 closed this Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static checking via feature(const_generics) and feature(const_evaluatable_checked)
2 participants