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 missing @since annotations for 4.00 .. 4.05 #916

Merged
merged 2 commits into from Feb 21, 2017

Conversation

Projects
None yet
9 participants
@edwintorok
Contributor

edwintorok commented Nov 14, 2016

Fixes https://caml.inria.fr/mantis/view.php?id=7412

After finding a few missing @SInCE annotations in the Unix module, I wrote the script below to find more (it is not guaranteed that it finds all missing annotations, but it found quite a few).

URL_PREFIX="https://github.com/ocaml/ocaml/commit"
check () {
    git diff --name-only $1..$2 -- "$3/*.mli" |\
    grep -Ev internal\|obj\|threadUnix\|raw | while read f; do
        echo "$f"
        rm -f commit_ids lines
        # find newly introduces functions/modules/exceptions/types
        git diff $1..$2 -- "$f" |\
            grep -Eo '^[+-] *(val|external|module|exception|type) [^:=*;]+'|\
            colrm 1 1|sed -re 's/[.]? *?$//'|sort|uniq -c|grep '^ *1'|\
            cut -f2 -d'1'|while read line; do
            echo " $line" >>lines
            # find the commit that introduced it
            # COMMIT=$(git log --pretty=format:%H -S "$line" -- "$f"|head -1)
            # echo "$COMMIT" >>commit_ids
        done
        if [ -f lines ]; then
            echo "$f"
            cat lines | while read line; do
                echo " $line"
                # find existing @since annotation if any
                grep "^ *$line" -A10 -- "$f" | tail -n +2| tr '\n' ' '|grep -Eo '^ *\(\*\*[^*]+@since[^*]+'|cut -f2 -d@
            done
            test -f commit_ids && sort -u commit_ids | while read id; do
                # unique commit ids for verification
                echo " $URL_PREFIX/$id";
                git tag --contains "$id" | head -1|while read tag; do
                    # @since tag based on commit (could be higher if cherry-picked and edited)
                    echo " @since $tag"
                done
            done
        fi
    done
}

VPREV=3.12.1
for V in 4.00.0 4.01.0 4.02.0 4.03.0 4.04.0 origin/trunk; do
    echo "------------------"
    echo "@since $V"
    for dir in otherlibs/* stdlib; do
        check "$VPREV" "$V" "$dir"
    done
    VPREV="$V"
done
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 14, 2016

Contributor

Thanks, this is very useful. I'm tempted to merge without checking each annotation one by one if you are reasonably confident in the methodology (and unless someone else objects to it, of course).

Contributor

alainfrisch commented Nov 14, 2016

Thanks, this is very useful. I'm tempted to merge without checking each annotation one by one if you are reasonably confident in the methodology (and unless someone else objects to it, of course).

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 14, 2016

Contributor

There's a little inconsistency of, e.g., 4.03 vs 4.03.0. Is it a good idea to put 4.05 before that's confirmed?

Contributor

dra27 commented Nov 14, 2016

There's a little inconsistency of, e.g., 4.03 vs 4.03.0. Is it a good idea to put 4.05 before that's confirmed?

Show outdated Hide outdated otherlibs/threads/thread.mli
@@ -40,7 +40,8 @@ val self : unit -> t
val id : t -> int
(** Return the identifier of the given thread. A thread identifier
is an integer that identifies uniquely the thread.
It can be used to build data structures indexed by threads. *)
It can be used to build data structures indexed by threads.
@since 4.03 *)

This comment has been minimized.

@gasche

gasche Nov 14, 2016

Member

This annotation is not quite right, Thread.id was added by ee63e8d back in 1996.

@gasche

gasche Nov 14, 2016

Member

This annotation is not quite right, Thread.id was added by ee63e8d back in 1996.

This comment has been minimized.

@edwintorok

edwintorok Nov 14, 2016

Contributor

indeed it used to be external id before, but I don't think that distinction matters for user code.

@edwintorok

edwintorok Nov 14, 2016

Contributor

indeed it used to be external id before, but I don't think that distinction matters for user code.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 14, 2016

Member

This scripts looks phenomenally useful. Which raises two questions:

  • Could you include it in tools/? We have other related scripts for quality analysis, eg. tools/check-typo
  • Is it possible to make it generic enough to work for user packages? The compiler distribution is far from the only library that would benefit from this. Maybe we could think of reimplementing it as an OCaml script to have a more convenient user-interface? (Not using bash is in no way mandatory, but in my personal experience as soon as I want to make something reusable I quickly need more robust abstraction facilities.)

@alainfrisch : You will call me a perfectionist, but I think that at least having a look at all functions to see if something looks odd would be warranted. I only looked at a few and found a couple errors, it would be interesting to iterate to fix the script.

Member

gasche commented Nov 14, 2016

This scripts looks phenomenally useful. Which raises two questions:

  • Could you include it in tools/? We have other related scripts for quality analysis, eg. tools/check-typo
  • Is it possible to make it generic enough to work for user packages? The compiler distribution is far from the only library that would benefit from this. Maybe we could think of reimplementing it as an OCaml script to have a more convenient user-interface? (Not using bash is in no way mandatory, but in my personal experience as soon as I want to make something reusable I quickly need more robust abstraction facilities.)

@alainfrisch : You will call me a perfectionist, but I think that at least having a look at all functions to see if something looks odd would be warranted. I only looked at a few and found a couple errors, it would be interesting to iterate to fix the script.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 14, 2016

Member

There's a little inconsistency of, e.g., 4.03 vs 4.03.0.

Indeed it would be better to use correct minor/bugfix versions when possible -- although if it's too painful, I think it's fine to merge without.

Is it a good idea to put 4.05 before that's confirmed?

In Batteries I use @since NEXT_RELEASE and grep and replace at release time. That said, while we don't know yet whether we will have a minor/bugfix release, I don't think that it should have any new standard library functions in it, so 4.05.0 is probably fine.

Member

gasche commented Nov 14, 2016

There's a little inconsistency of, e.g., 4.03 vs 4.03.0.

Indeed it would be better to use correct minor/bugfix versions when possible -- although if it's too painful, I think it's fine to merge without.

Is it a good idea to put 4.05 before that's confirmed?

In Batteries I use @since NEXT_RELEASE and grep and replace at release time. That said, while we don't know yet whether we will have a minor/bugfix release, I don't think that it should have any new standard library functions in it, so 4.05.0 is probably fine.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Nov 14, 2016

Contributor

Is it possible to make it generic enough to work for user packages?

Except maybe for larger projects I think that for user packages you rather want to simply use the documentation that corresponds to the smallest version of the package you code against. odigging your installed packages gives you just that.

Contributor

dbuenzli commented Nov 14, 2016

Is it possible to make it generic enough to work for user packages?

Except maybe for larger projects I think that for user packages you rather want to simply use the documentation that corresponds to the smallest version of the package you code against. odigging your installed packages gives you just that.

@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok Nov 14, 2016

Contributor

@gasche I don't think there is any need to rush in merging this PR, I can iterate/improve the script if you want this in tools/, and go through the patch once more by hand. Regex based match won't be perfect, (but I don't want to turn this into an overkil by looking at ocamlobjinfo output or something).
@dra27 which form is preferred for @SInCE? I've seen both used in existing documentation, I tend to prefer the full version number form.

Contributor

edwintorok commented Nov 14, 2016

@gasche I don't think there is any need to rush in merging this PR, I can iterate/improve the script if you want this in tools/, and go through the patch once more by hand. Regex based match won't be perfect, (but I don't want to turn this into an overkil by looking at ocamlobjinfo output or something).
@dra27 which form is preferred for @SInCE? I've seen both used in existing documentation, I tend to prefer the full version number form.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 14, 2016

Member

I definitely think that it would be good to have the script in tools/, because it would make it easier to distribute its usage (and the responsibility to use it) among contributors in the future. It's of course fine if the output is imperfect -- and for future runs the output should be much shorter, so having a human review it would not be an issue. (You could even have the script write a small message pointing out that the output is known to be imperfect, if you would feel guilty about it otherwise.) The goal of the current review/iteration should not be to have a "perfect output", but to find among the inevitable glitches all the one that you can relatively easily solve programmatically.

I don't think there are strong conventions on whether you should use @since X.Y.Z or just @since X.Y, but I think that the former should be preferred if it does not make the script noticeably more complex. In particular, I'm sure some older releases added functions in X.Y.Z that were not in available X.Y.0.

Member

gasche commented Nov 14, 2016

I definitely think that it would be good to have the script in tools/, because it would make it easier to distribute its usage (and the responsibility to use it) among contributors in the future. It's of course fine if the output is imperfect -- and for future runs the output should be much shorter, so having a human review it would not be an issue. (You could even have the script write a small message pointing out that the output is known to be imperfect, if you would feel guilty about it otherwise.) The goal of the current review/iteration should not be to have a "perfect output", but to find among the inevitable glitches all the one that you can relatively easily solve programmatically.

I don't think there are strong conventions on whether you should use @since X.Y.Z or just @since X.Y, but I think that the former should be preferred if it does not make the script noticeably more complex. In particular, I'm sure some older releases added functions in X.Y.Z that were not in available X.Y.0.

@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok Dec 9, 2016

Contributor

Sorry for the delay. I have updated my PR, changes since last time:

  • rebased on latest trunk
  • added tools/lintapidiff.{ml,sh}
    • works on a Parsetree (Env.fold* would be nice, but it is harder to extract the module annotations). might've exagerated a bit with this, maybe this is not what you meant by 'reimplement in OCaml'
    • assumptions
      • you are running the tool on a development branch that is newer than any other tagged release (otherwise it complains that you deleted a bunch of values without marking them as deprecated first)
      • you have OCaml and findlib already installed (it is not integrated into the compiler's build system)
      • you are running the tool in a git checkout
      • version numbers are of form major.minor or major.minor.patch
      • if a value is marked as deprecated no further error is reported
      • @since/@deprecated annotations are considered to be "inherited" from the parent module (although when reporting you get one report for each value)
      • @SInCE annotations can be extract from ocaml.doc attributes using a regex
      • missing annotations on undocumented symbols of undocumented modules are not reported
      • it analyzes all tagged releases since 3.0: takes ~11s in total, YMMV (needed to correctly report @SInCE mismatches for now)
      • its scope is limited to that of this PR (analyzes only values, types and exceptions; reports only on missing @SInCE for >= 4.00.0). Tried to look at type constructors too, but there are too many false positives from Bigarray's GADTs (which I assume shouldn't be all annotated)
    • only looks at newly added, deleted symbols and mismatched @SInCE. Detecting other ways of breaking an API is out of scope.
    • running on trunk shows these problems
  • fixed the problems reported by the above tool
    • incorrect @SInCE in my previous PR on Hashtbl.hash_param, Format.get_formatter_output_functions, Thread.id
    • add annotations for Bigarray.Array1.slice, Bigarray.genarray_of_array0, Format.formatter_out_functions, Hashtbl.*.{find_opt, filter_map_inplace,reset}, Hashtbl.{stats,statistics}, StringLabels.init, Sys.backend_type
  • use major.minor.patch format for newly added @SInCE annotations consistently

If you prefer I can put the tool into a separate PR, it can probably still be improved.

Contributor

edwintorok commented Dec 9, 2016

Sorry for the delay. I have updated my PR, changes since last time:

  • rebased on latest trunk
  • added tools/lintapidiff.{ml,sh}
    • works on a Parsetree (Env.fold* would be nice, but it is harder to extract the module annotations). might've exagerated a bit with this, maybe this is not what you meant by 'reimplement in OCaml'
    • assumptions
      • you are running the tool on a development branch that is newer than any other tagged release (otherwise it complains that you deleted a bunch of values without marking them as deprecated first)
      • you have OCaml and findlib already installed (it is not integrated into the compiler's build system)
      • you are running the tool in a git checkout
      • version numbers are of form major.minor or major.minor.patch
      • if a value is marked as deprecated no further error is reported
      • @since/@deprecated annotations are considered to be "inherited" from the parent module (although when reporting you get one report for each value)
      • @SInCE annotations can be extract from ocaml.doc attributes using a regex
      • missing annotations on undocumented symbols of undocumented modules are not reported
      • it analyzes all tagged releases since 3.0: takes ~11s in total, YMMV (needed to correctly report @SInCE mismatches for now)
      • its scope is limited to that of this PR (analyzes only values, types and exceptions; reports only on missing @SInCE for >= 4.00.0). Tried to look at type constructors too, but there are too many false positives from Bigarray's GADTs (which I assume shouldn't be all annotated)
    • only looks at newly added, deleted symbols and mismatched @SInCE. Detecting other ways of breaking an API is out of scope.
    • running on trunk shows these problems
  • fixed the problems reported by the above tool
    • incorrect @SInCE in my previous PR on Hashtbl.hash_param, Format.get_formatter_output_functions, Thread.id
    • add annotations for Bigarray.Array1.slice, Bigarray.genarray_of_array0, Format.formatter_out_functions, Hashtbl.*.{find_opt, filter_map_inplace,reset}, Hashtbl.{stats,statistics}, StringLabels.init, Sys.backend_type
  • use major.minor.patch format for newly added @SInCE annotations consistently

If you prefer I can put the tool into a separate PR, it can probably still be improved.

@gasche

gasche approved these changes Dec 10, 2016

I did a review to make sure that I would be comfortable adding the code to the compiler distribution. It is nitpicky, mostly as a way to force myself to read the code. I agree that the design is reasonably solid and I like this implementation approach.

(One disadvantage over the more greppy previous approach is that it won't work very well for code that is not immediately valid OCaml code, eg. if it uses a cpp-like preprocessor, but I think people can adjust their build to run the tool after preprocessing in this case.)

Show outdated Hide outdated tools/lintapidiff.ml
open Location
open Parsetree
module IdMap = Depend.StringMap

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

Is there any particular reason to use Depend.StringMap instead of Map.Make(String) here? I would find the latter more clear as to what the interface is, and I don't immediately see why you chose to reference Depend.

@gasche

gasche Dec 10, 2016

Member

Is there any particular reason to use Depend.StringMap instead of Map.Make(String) here? I would find the latter more clear as to what the interface is, and I don't immediately see why you chose to reference Depend.

This comment has been minimized.

@edwintorok

edwintorok Dec 10, 2016

Contributor

I was looking at the code of depend.ml to learn how to use the parsetree, I'll remove it.

@edwintorok

edwintorok Dec 10, 2016

Contributor

I was looking at the code of depend.ml to learn how to use the parsetree, I'll remove it.

let find_attr lst attrs =
try Some (List.find (fun (loc, _) -> List.mem loc.txt lst) attrs)
with Not_found -> None

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

Nitpicking, but match List.find ... with exception Not_found -> None | attr -> Some attr is a better style nowadays. Don't change it here if this one is more natural to you -- also having code that works on older OCaml version might make sense for this program.

@gasche

gasche Dec 10, 2016

Member

Nitpicking, but match List.find ... with exception Not_found -> None | attr -> Some attr is a better style nowadays. Don't change it here if this one is more natural to you -- also having code that works on older OCaml version might make sense for this program.

This comment has been minimized.

@edwintorok

edwintorok Dec 10, 2016

Contributor

Yep, I used this once below when I needed tail recursion, but otherwise I'm still used to the old way.

@edwintorok

edwintorok Dec 10, 2016

Contributor

Yep, I used this once below when I needed tail recursion, but otherwise I'm still used to the old way.

Show outdated Hide outdated tools/lintapidiff.ml
with Not_found -> false
let get parent_info loc attrs =
let doc = get_doc ["ocaml.doc"; "ocaml.text"] attrs in

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

I wonder, shouldn't we also support unqualified doc and text? Are such explicitly-user-written attributes never considered by ocamldoc?

@gasche

gasche Dec 10, 2016

Member

I wonder, shouldn't we also support unqualified doc and text? Are such explicitly-user-written attributes never considered by ocamldoc?

This comment has been minimized.

@Octachron

Octachron Dec 10, 2016

Contributor

Ocamldoc does not use documentation attributes currently: their generation by the lexer is even disabled by ocamldoc since #348. (I also think it might be a good idea to support unqualified doc and text.)

@Octachron

Octachron Dec 10, 2016

Contributor

Ocamldoc does not use documentation attributes currently: their generation by the lexer is even disabled by ocamldoc since #348. (I also think it might be a good idea to support unqualified doc and text.)

Show outdated Hide outdated tools/lintapidiff.ml
match get_doc ["ocaml.text"] attrs with (* for toplevel module annotation *)
| None -> false
| Some text ->
try Misc.search_substring "@deprecated" text 0 >= 0;

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

Nitpicking: the semicolon here is mildly confusing (usually people use it to indicate that a value is of type unit, not that it is ignored) and would break with -strict-sequence.

@gasche

gasche Dec 10, 2016

Member

Nitpicking: the semicolon here is mildly confusing (usually people use it to indicate that a value is of type unit, not that it is ignored) and would break with -strict-sequence.

Show outdated Hide outdated tools/lintapidiff.ml
deprecated = parent_info.deprecated || is_deprecated attrs;
loc;
has_doc_parent = parent_info.has_doc;
has_doc = doc <> None

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

What is has_doc_parent used for? One thing that is a bit strange in this code is that the other attributes will behave nicely if there are several nested parents, but these two attributes will only refer to the immediate parent and not the aggregated result on all parents. In particular, if I understand correctly, a undocumented value in an undocumented module in a documented module will be consider undocumented.

@gasche

gasche Dec 10, 2016

Member

What is has_doc_parent used for? One thing that is a bit strange in this code is that the other attributes will behave nicely if there are several nested parents, but these two attributes will only refer to the immediate parent and not the aggregated result on all parents. In particular, if I understand correctly, a undocumented value in an undocumented module in a documented module will be consider undocumented.

This comment has been minimized.

@edwintorok

edwintorok Dec 10, 2016

Contributor

Making has_doc_parent fully propagated (i.e. parent_info.has_doc_parent || parent_info.has_doc) produces a lot of noise for MoreLabels, although I could easily just exclude morelabels from checking, and enable full propagation.

@edwintorok

edwintorok Dec 10, 2016

Contributor

Making has_doc_parent fully propagated (i.e. parent_info.has_doc_parent || parent_info.has_doc) produces a lot of noise for MoreLabels, although I could easily just exclude morelabels from checking, and enable full propagation.

Show outdated Hide outdated tools/lintapidiff.sh
#!/bin/sh
set -e
TAGS=$(git tag|grep '^[0-9]*.[0-9]*.[0-9]*$'|grep -v '^[12].')
ocamlfind ocamlopt -package compiler-libs.common,str tools/lintapidiff.ml \

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

I see that you documented the assumption that this tool relies on an existing installation with findlib, but is there any particular reason for this, or just that you found it easier to iterate on it as a separate tool? I think the reasoning can go both ways: if it's in tools it would be consistent to link with the compiler distribution directly, but having it external like this makes it easier for other people to just steal the scripts and use them for their projects. I like that second advantage and I suspect it could also benefit some other tools, so I would leave things the way you created them.

@gasche

gasche Dec 10, 2016

Member

I see that you documented the assumption that this tool relies on an existing installation with findlib, but is there any particular reason for this, or just that you found it easier to iterate on it as a separate tool? I think the reasoning can go both ways: if it's in tools it would be consistent to link with the compiler distribution directly, but having it external like this makes it easier for other people to just steal the scripts and use them for their projects. I like that second advantage and I suspect it could also benefit some other tools, so I would leave things the way you created them.

This comment has been minimized.

@edwintorok

edwintorok Dec 10, 2016

Contributor

Initially this had #use and #require (easier to edit/run from within Emacs). I could move this to the Makefile (and not built it by default, just have a target you can run that builds and runs the linter).

@edwintorok

edwintorok Dec 10, 2016

Contributor

Initially this had #use and #require (easier to edit/run from within Emacs). I could move this to the Makefile (and not built it by default, just have a target you can run that builds and runs the linter).

Show outdated Hide outdated tools/lintapidiff.ml
module Diff = struct
type seen_info = {
not_seen: version option;

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

Where is not_seen set? I understand from the report that it identifies a version in which the item is missing (I would thus find option list more natural), but it seems to be None everywhere in the code.

@gasche

gasche Dec 10, 2016

Member

Where is not_seen set? I understand from the report that it identifies a version in which the item is missing (I would thus find option list more natural), but it seems to be None everywhere in the code.

This comment has been minimized.

@edwintorok

edwintorok Dec 10, 2016

Contributor

In parse_file_at_rev merge: 20e6a66#diff-8de361ea648ec4912974450f0d705a9fR193. I think last_not_seen, would be a better name.

@edwintorok

edwintorok Dec 10, 2016

Contributor

In parse_file_at_rev merge: 20e6a66#diff-8de361ea648ec4912974450f0d705a9fR193. I think last_not_seen, would be a better name.

Show outdated Hide outdated tools/lintapidiff.ml
| None, None -> assert false
| _, Some {has_doc_parent=false;has_doc=false;_} -> None (* undocumented *)
| Some {deprecated=true;first_seen;_}, None -> None (* deleted deprecated *)
| Some _, None ->

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

In the case above {deprecated=true;_} should suffice. In the present case I think {deprecated=false;_} would be clearer -- it's best to avoid relying on case ordering when it is easy.

@gasche

gasche Dec 10, 2016

Member

In the case above {deprecated=true;_} should suffice. In the present case I think {deprecated=false;_} would be clearer -- it's best to avoid relying on case ordering when it is easy.

This comment has been minimized.

@edwintorok

edwintorok Dec 10, 2016

Contributor

first_seen in {deprecated=true;first_seen;_} was required for disambiguation, although I can see how thats a bit ugly, I'll add a type annotation instead.

@edwintorok

edwintorok Dec 10, 2016

Contributor

first_seen in {deprecated=true;first_seen;_} was required for disambiguation, although I can see how thats a bit ugly, I'll add a type annotation instead.

Show outdated Hide outdated tools/lintapidiff.ml
| Some {deprecated=true;first_seen;_}, None -> None (* deleted deprecated *)
| Some _, None ->
Some (default, "deleted non-deprecated", seen, latest)
| _, Some {deprecated=true;since=None;_} -> None (* marked as deprecated *)

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

Isn't it also a problem if deprecated items don't have a @since marker?

@gasche

gasche Dec 10, 2016

Member

Isn't it also a problem if deprecated items don't have a @since marker?

This comment has been minimized.

@edwintorok

edwintorok Dec 10, 2016

Contributor

I think that if something is marked as deprecated users would be better of avoiding to use it, than figuring out the required compiler version constraints where they can use it. e.g. Array.make_float was rather short-lived: introduced in 4.02.0, then marked as deprecated, removing the since annotation.

@edwintorok

edwintorok Dec 10, 2016

Contributor

I think that if something is marked as deprecated users would be better of avoiding to use it, than figuring out the required compiler version constraints where they can use it. e.g. Array.make_float was rather short-lived: introduced in 4.02.0, then marked as deprecated, removing the since annotation.

Show outdated Hide outdated tools/lintapidiff.ml
| Some {first_seen;_}, Some {since=None;_} when is_old first_seen ->
None (* OK, very old *)
| Some {first_seen;_}, Some {loc; since=None;_} ->
Some (loc, "missing @since", seen, latest)

This comment has been minimized.

@gasche

gasche Dec 10, 2016

Member

I think the when + case ordering between two clauses is less readable than having a single clause with a conditional test.

@gasche

gasche Dec 10, 2016

Member

I think the when + case ordering between two clauses is less readable than having a single clause with a conditional test.

@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok Dec 10, 2016

Contributor

Thanks a lot for the review. I appreciate nitpicky comments, and pushed some commits to address most of them (I haven't replied to each comment individually, but see the commit message in the fixup commits).

Contributor

edwintorok commented Dec 10, 2016

Thanks a lot for the review. I appreciate nitpicky comments, and pushed some commits to address most of them (I haven't replied to each comment individually, but see the commit message in the fixup commits).

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 12, 2016

Contributor
Contributor

shindere commented Dec 12, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 12, 2016

Member

@damiendoligez may know about the policy. @damiendoligez, I would also be interested in your general opinion on the patch. I personally think that it is good and that we should consider merging it.

@edwintorok thanks for the fixups. I agree with your justifications in the no-change cases.

Member

gasche commented Dec 12, 2016

@damiendoligez may know about the policy. @damiendoligez, I would also be interested in your general opinion on the patch. I personally think that it is good and that we should consider merging it.

@edwintorok thanks for the fixups. I agree with your justifications in the no-change cases.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Feb 14, 2017

Member

I like the idea of this patch (both the tool and the fixes) although I don't have time to review (and I trust @gasche for that).

The copyright header is mandatory on all source files. @edwintorok have you signed the CLA already for some other contribution?

Member

damiendoligez commented Feb 14, 2017

I like the idea of this patch (both the tool and the fixes) although I don't have time to review (and I trust @gasche for that).

The copyright header is mandatory on all source files. @edwintorok have you signed the CLA already for some other contribution?

@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok Feb 14, 2017

Contributor

@damiendoligez I haven't signed a CLA yet. Can I contact you via e-mail to discuss the details of signing that please?

Contributor

edwintorok commented Feb 14, 2017

@damiendoligez I haven't signed a CLA yet. Can I contact you via e-mail to discuss the details of signing that please?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 14, 2017

Member

@edwintorok it is of course fine to contact Damien if you have further questions, but there is an explanation on the CLA in the CONTRIBUTING.md file, both of what it is (and why) and what is the process to complete it (essentially the process is to print a document, sign it, scan it back, and send it to Xavier Leroy by email).

Member

gasche commented Feb 14, 2017

@edwintorok it is of course fine to contact Damien if you have further questions, but there is an explanation on the CLA in the CONTRIBUTING.md file, both of what it is (and why) and what is the process to complete it (essentially the process is to print a document, sign it, scan it back, and send it to Xavier Leroy by email).

@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok Feb 17, 2017

Contributor

@gasche I have rebased the branch, fixed some new warnings from lintapidiff, and added the copyright header on lintapidiff.ml. Please let me know if these changes are ok, I'll then squash the commits marked with squash!

Contributor

edwintorok commented Feb 17, 2017

@gasche I have rebased the branch, fixed some new warnings from lintapidiff, and added the copyright header on lintapidiff.ml. Please let me know if these changes are ok, I'll then squash the commits marked with squash!

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 21, 2017

Contributor

CLA received in good order. I don't know what the next step is, but if anyone does, please proceed.

Contributor

xavierleroy commented Feb 21, 2017

CLA received in good order. I don't know what the next step is, but if anyone does, please proceed.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 21, 2017

Member

I think this should be merged in trunk and 4.05, and I can take care of it. @edwintorok, could you squash and also add a Changes entry (this was a sizeable amount of work so it deserves one)?

Member

gasche commented Feb 21, 2017

I think this should be merged in trunk and 4.05, and I can take care of it. @edwintorok, could you squash and also add a Changes entry (this was a sizeable amount of work so it deserves one)?

@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok Feb 21, 2017

Contributor

Thanks, squashed it down to 2 commits and added the Changes entry.

Contributor

edwintorok commented Feb 21, 2017

Thanks, squashed it down to 2 commits and added the Changes entry.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 21, 2017

Member

(Hm, I was only thinking of squashing the squash! commit and you squashed a bit more. That's fine.)

The commit message in 640bc8b and the comment in lintapidiff.ml still mention a lintapidiff.sh script, but as far as I can tell this script does not exist in the current version of the patch.

Member

gasche commented Feb 21, 2017

(Hm, I was only thinking of squashing the squash! commit and you squashed a bit more. That's fine.)

The commit message in 640bc8b and the comment in lintapidiff.ml still mention a lintapidiff.sh script, but as far as I can tell this script does not exist in the current version of the patch.

edwintorok added some commits Dec 8, 2016

Documentation: improve @SInCE annotations
Add missing @SInCE annotations for OCaml versions 4.00.0 - 4.05.0,
and fix existing annotations as needed:

Format.ikprintf: clarify ambiguity on @SInCE 4.0 annotation
See b815196

Hashtbl.is_randomized and ListLabels.sort_uniq should be @SInCE 4.03
List.sort_uniq is 4.02 but ListLabels.sort_uniq is 4.03
See:
512d128
189d29b
Documentation tool: add tools/lintapidiff.ml
Run 'make lintapidiff' in the root of a git checkout to get a list of
potentially missing or wrong @SInCE annotations.

The tool is not built by default, you have to first run 'make
world.opt', and then run 'make lintapidiff'.

lintapidiff doesn't support stop comments: add explicit list of changes to ignore.

see copyright header for license.
@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok Feb 21, 2017

Contributor

Thanks for spotting that, pushed updated branch.

Contributor

edwintorok commented Feb 21, 2017

Thanks for spotting that, pushed updated branch.

@gasche gasche merged commit 4f46291 into ocaml:trunk Feb 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 21, 2017

Member

Merged, and pushed to 4.05 as well. Thanks!

Member

gasche commented Feb 21, 2017

Merged, and pushed to 4.05 as well. Thanks!

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 2, 2017

Member

Note: we just used lintapidiff in preparation for the 4.06, and it was useful (we caught a missing @since in stdlib/uchar.mli). Thanks again!

Member

gasche commented Oct 2, 2017

Note: we just used lintapidiff in preparation for the 4.06, and it was useful (we caught a missing @since in stdlib/uchar.mli). Thanks again!

@edwintorok edwintorok deleted the edwintorok:add-missing-since-annots branch Oct 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment