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

Add MPR#6975: truncate in Buffer module #902

Merged
merged 11 commits into from Nov 17, 2016
Merged

Add MPR#6975: truncate in Buffer module #902

merged 11 commits into from Nov 17, 2016

Conversation

dc-mak
Copy link
Contributor

@dc-mak dc-mak commented Nov 9, 2016

As requested by this Mantis request.
Since the intention of truncate is specifically to shorten, an argument greater than the current buffer length is seen as an error - it is the responsibility of the caller to ensure this is not the case.

@@ -131,3 +131,8 @@ val add_channel : t -> in_channel -> int -> unit
val output_buffer : out_channel -> t -> unit
(** [output_buffer oc b] writes the current contents of buffer [b]
on the output channel [oc]. *)

val truncate : t -> int -> unit
(** [truncate b len] truncates the length of [b] to
Copy link
Member

@gasche gasche Nov 9, 2016

Choose a reason for hiding this comment

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

to [len]?

val truncate : t -> int -> unit
(** [truncate b len] truncates the length of [b] to
Note: it does not change the size of the underlying buffer.
Raise [Invalid_argument] if [len < 0] or [len > b.position]. *)
Copy link
Contributor

@alainfrisch alainfrisch Nov 9, 2016

Choose a reason for hiding this comment

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

b.position is a bit opaque for users; you could use len > Buffer.length b.


val truncate : t -> int -> unit
(** [truncate b len] truncates the length of [b] to
Note: it does not change the size of the underlying buffer.
Copy link
Contributor

@alainfrisch alainfrisch Nov 9, 2016

Choose a reason for hiding this comment

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

There is no other reference to the "underlying buffer" in the current doc. Perhaps "the internal byte sequence is not shortened", or "no memory is reclaimed"?


let truncate b len =
if len < 0 || len > b.position then
invalid_arg "Buffer.truncate"
Copy link
Contributor

@alainfrisch alainfrisch Nov 9, 2016

Choose a reason for hiding this comment

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

The style is to use 2-space indentations.

@alainfrisch
Copy link
Contributor

alainfrisch commented Nov 9, 2016

I'm in favor of this proposal (and so was @damiendoligez according to the Mantis ticket). Please add a unit test and a Changes entry.


val truncate : t -> int -> unit
(** [truncate b len] truncates the length of [b] to [len]
Note: the intenal byte sequence is not shortenend.
Copy link
Member

@gasche gasche Nov 11, 2016

Choose a reason for hiding this comment

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

internal

Copy link
Contributor

@mshinwell mshinwell Nov 11, 2016

Choose a reason for hiding this comment

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

And "shortened"

Copy link
Contributor Author

@dc-mak dc-mak Nov 15, 2016

Choose a reason for hiding this comment

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

Yes, on them (and test cases) now.

Copy link
Contributor Author

@dc-mak dc-mak Nov 15, 2016

Choose a reason for hiding this comment

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

Actually, speaking of test cases, can you point me to a canonical example of what a good test looks like? The examples in the tests directory are inconsistent, follow their own conventions, duplicate many functions, etc. across different modules/directories.

Copy link
Member

@gasche gasche Nov 15, 2016

Choose a reason for hiding this comment

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

One thing you can do to avoid existential worries is to add your tests to a file already testing buffer, namely basic-more/tbuffer.ml.

In general tests are done by having tests print some output (either by running the program, or the compiler or toplevel on the program), and comparing it to a reference output. The best outputs, I think, are those which give useful information when they fail: basic-more/tbuffer.ml just prints the list of test numbers that pass or fail, it's not very useful compared to lib-printf/pr6534.reference which gives useful information out failure. But that is a second-order concern: just having a good test that fails at the right time is the important thing here, and being locally consistent with wherever you decide to add your tests suffices. If you don't find a place you like, you can create tests/lib-buffer, which would be the natural place for Buffer test, and invent your own perfect convention.

val truncate : t -> int -> unit
(** [truncate b len] truncates the length of [b] to [len]
Note: the intenal byte sequence is not shortenend.
Raise [Invalid_argument] if [len < 0] or [len > b.position]. *)
Copy link
Member

@yallop yallop Nov 15, 2016

Choose a reason for hiding this comment

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

b.position is an implementation detail, not exposed in the interface. Better:

or [len > length b]

### Other libraries:

- MPR#7339, GPR#787: Support the '0 dimension' case for bigarrays
(see Bigarray documentation)
(Laurent Mazare,
review by Gabriel Scherer, Alain Frisch and Hezekiah M. Carty)
>>>>>>> upstream/trunk
Copy link
Member

@gasche gasche Nov 16, 2016

Choose a reason for hiding this comment

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

typo

@@ -54,6 +54,9 @@ Next version (4.05.0):
(Gabriel de Perthuis, with contributions from Alain Frisch, review by
Hezekiah M. Carty and Simon Cruanes, initial report by Gerd Stolpmann)

- MPR#6975: Truncate function added to stdlib Buffer module
Copy link
Member

@gasche gasche Nov 16, 2016

Choose a reason for hiding this comment

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

While we are at it (sorry for the endless nit-picking): as mentioned in the contributing guidelines, you should include both mantis and github numbers: PR#6975, GPR#902 (for now the convention is still PR# and not MPR#), and it should be at the top of the "Standard library" section because we order by mantis PR numbers first, so entries without a mantis ticket are last.

Copy link
Contributor Author

@dc-mak dc-mak Nov 17, 2016

Choose a reason for hiding this comment

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

Oh not at all, in fact, (as a first timer) I'm sorry for all the mistakes - I'm really grateful for and appreciate the patience shown by everyone who's commented.

@gasche
Copy link
Member

gasche commented Nov 16, 2016

I think it's good and would be willing to merge. My inner unit tester is telling me that two natural cases to test (that seem to be missing currently) would be resetting at the current length, and resetting at length 0; but I might be able to overcome perfectionism and merge without them.

@gasche gasche merged commit d5af984 into ocaml:trunk Nov 17, 2016
2 checks passed
@gasche
Copy link
Member

gasche commented Nov 17, 2016

I squashed and merged, thanks for the nice PR!

@dc-mak dc-mak deleted the buffer_truncate branch Nov 17, 2016
camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017
MPR#6975: add truncate in Buffer module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants