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

Expose st_dev and st_ino fields of stat structure #1589

Merged
merged 2 commits into from
Feb 16, 2017

Conversation

zevlg
Copy link
Contributor

@zevlg zevlg commented Feb 15, 2017

Makes possible to access device and inode value from pony's FileInfo type

@@ -69,6 +71,8 @@ static void windows_stat(const char* path, pony_stat_t* p, struct __stat64* st,
p->broken = false;

p->hard_links = (uint32_t)st->st_nlink;
p->device = (uint32_t)st->st_dev;
Copy link
Member

@jemc jemc Feb 15, 2017

Choose a reason for hiding this comment

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

According to @dipinhora's PR, it looks like the st_dev type (dev_t) should be int64_t on Linux, and int32_t on OSX.

Perhaps that means we should be using int64_t here (to be large enough for either system)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

negative device number? thats insane! It should not be signed for sure, maybe uint64_t ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't actually image system with 4294967296 devices :). However no one could imagine so many internet peers when IPv4 was designed

So uint64_t is reasonable enough, should I make a change ?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't investigated myself, so maybe we can get @dipinhora's input.

Copy link
Contributor

Choose a reason for hiding this comment

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

I figured out the different types using clang -E test.c | grep <TYPE> or gcc -E test.c | grep <TYPE> and following the chain of types to get to the underlying machine type. test.c included every posix header file and declared a variable of each type. I did this on both OSX and Linux (ubuntu in a VM) both on my laptop.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this on the sync call, and agreed that we should use uint64_t/U64.

@jemc jemc requested a review from dipinhora February 15, 2017 17:16
@jemc jemc removed the request for review from dipinhora February 15, 2017 22:02
Copy link
Member

@jemc jemc left a comment

Choose a reason for hiding this comment

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

Looks good to me, and can be merged when it passes CI.

@jemc
Copy link
Member

jemc commented Feb 15, 2017

We want to merge this before doing the 0.11.0 release (#1543).

@Praetonus Praetonus added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Feb 16, 2017
@Praetonus Praetonus merged commit 89007c7 into ponylang:master Feb 16, 2017
ponylang-main added a commit that referenced this pull request Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants