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

[RDY] Implement `FileID` struct #775

Merged
merged 11 commits into from Jun 28, 2014

Conversation

Projects
None yet
4 participants
@stefan991
Copy link
Contributor

stefan991 commented May 28, 2014

FileID encapsulates st_dev and st_ino. It is a new abstraction used to check if two files are the same. FileIDs are embedded inside other struts like buf_t or ff_visited_T, where a full FileInfo would be to big.

All uses of stat.st_dev and stat.st_ino outside of src/os/ are replaced with FileInfo/FileID.
The only exception is memline.c, which needs the inode for the block0 used in swapfiles.

This continues #619.

@philix

This comment has been minimized.

Copy link
Member

philix commented May 28, 2014

LGTM. All the little issues I found have been clarified or addressed by @stefan991.

This PR solves a well defined code smell: Primitive Obsession. I'm using this as an example in [1].

[1] https://github.com/neovim/neovim/wiki/C-Refactorings-and-Code-Smells-Catalog

@stefan991

This comment has been minimized.

Copy link
Contributor

stefan991 commented May 29, 2014

Updated according to @philix comments, rebased on top of master, tagging [RDY]

@philix Thanks for your input 👍

@stefan991 stefan991 changed the title [RFC] Implement `FileID` struct [RDY] Implement `FileID` struct May 29, 2014

/// Struct which encapsulates inode/dev_id information.
typedef struct {
uint64_t inode; ///< @private The inode of the file
uint64_t device_id; ///< @private The id of the device containing the file

This comment has been minimized.

@Hinidu

Hinidu May 30, 2014

Contributor

Why not to define these structs in fs.c and just place forward declarations and typedefs in header? This will do their fields really private.

This comment has been minimized.

@stefan991

stefan991 May 30, 2014

Contributor

I thought about that, but it would not be possible to use this struct without pointers outside of fs.c. E.g. embedding in other structs would not be possible.

This comment has been minimized.

@Hinidu

Hinidu May 30, 2014

Contributor

👍 I didn't notice that you use it in such ways.

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 25, 2014

@stefan991 can you rebase again? @justinmk this PR looks good. A good candidate for merge from the huge list of unmerged PRs.

@stefan991

This comment has been minimized.

Copy link
Contributor

stefan991 commented Jun 25, 2014

@philix this is already rebased :)

stefan991 added some commits May 10, 2014

FileID: implement `FileID` struct
`FileID` should encapsulate `st_dev` and `st_ino`. It is a new abstraction
used to check if two files are the same. `FileID`s will be embeded inside
other struts like `buf_t` or `ff_visited_T`, where a full `FileInfo` would be
to big.
FileID: remove last use of `st_ino` in memline.c
* FileID can’t be used in memline.c, because the block0 is defined to
use only a 32bit ino.
* implemented `os_file_info_get_inode`
* deprecated `os_file_info_get_inode

@justinmk justinmk merged commit 147ab48 into neovim:master Jun 28, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

justinmk added a commit that referenced this pull request Jun 28, 2014

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 28, 2014

Very nice!

@justinmk

This comment has been minimized.

Copy link

justinmk commented on src/nvim/if_cscope_defs.h in 7340f61 Jun 28, 2014

should have comment: "file id of cscope db"

Grimy pushed a commit to Grimy/neovim that referenced this pull request Jan 7, 2015

dwb pushed a commit to dwb/neovim that referenced this pull request Feb 21, 2017

Refactor proselint maker(s) neovim#390 (neovim#775)
 - remove necessary checks and add %W to proselint.
 - add abort to neomake#makers#ft#tex#proselint()
 - move proselint maker to neomake#makers#ft#text#proselint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment