Skip to content

Don't use len() in the virtual IO setup#135

Merged
mgeier merged 1 commit intomasterfrom
issue-135
May 18, 2015
Merged

Don't use len() in the virtual IO setup#135
mgeier merged 1 commit intomasterfrom
issue-135

Conversation

@bastibe
Copy link
Owner

@bastibe bastibe commented May 18, 2015

Currently, we have this:

def vio_get_filelen(user_data):
    # first try __len__(), if not available fall back to seek()/tell()
    try:
        size = len(file)
    except TypeError:
        curr = file.tell()
        file.seek(0, SEEK_END)
        size = file.tell()
        file.seek(curr, SEEK_SET)
    return size

I suspect that the use of len() is a now obsolete artifact from the development in #1.
It was added there to seemingly support "streaming", but it turned out that this is not supported by the virtual IO call anyway.

Every supported file-like object must have the methods seek() and tell() anyway, so I think its superfluous to check with len() first.

Can we get rid of this remnant from the past?

I guess the cleaned-up version would look like this:

def vio_get_filelen(user_data):
    curr = file.tell()
    file.seek(0, SEEK_END)
    size = file.tell()
    file.seek(curr, SEEK_SET)
    return size

I'm wondering why get_filelen is even part of libsndfile's API, since it can be easily implemented with seek and tell ...?

@bastibe
Copy link
Owner

bastibe commented May 6, 2015

There are cases where len actually does the right thing. I would like to keep len for that reason.

@mgeier
Copy link
Contributor Author

mgeier commented May 11, 2015

There are cases where len actually does the right thing.

Sure, it might do the right thing, that's not the point.
The question rather is: Is there a conceivable situation where len() does something that cannot be equally well done with seek() and tell()?

Since seek() and tell() are always available, I don't see any necessity for __len__().

The only reason I could theoretically think of, is if __len__() is more efficient than seek() + tell().
But this only happens if the implementation of the file-like object is broken.

But there might be an error in my reasoning ...

@bastibe
Copy link
Owner

bastibe commented May 15, 2015

I'm not convinced. But it makes the code cleaner. If you want to change it, go ahead.

@mgeier mgeier self-assigned this May 16, 2015
@mgeier mgeier changed the title Don't use len() in the virtual IO setup? Don't use len() in the virtual IO setup May 16, 2015
mgeier added a commit that referenced this pull request May 16, 2015
@mgeier mgeier added this to the 0.7.1 milestone May 16, 2015
@bastibe
Copy link
Owner

bastibe commented May 18, 2015

Can we merge this?

@mgeier mgeier merged commit dc8b6a8 into master May 18, 2015
@mgeier mgeier deleted the issue-135 branch May 18, 2015 17:15
@mgeier
Copy link
Contributor Author

mgeier commented May 18, 2015

Yes, I rebased and merged.

@mgeier mgeier modified the milestones: 0.7.1, 0.8.0 Oct 2, 2015
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.

2 participants