-
Notifications
You must be signed in to change notification settings - Fork 89
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
Collect occurrences information #976
Conversation
2c2fe55
to
372cee0
Compare
I added path to constructors and their resolving function to the PR. I'd particularly appreciate a review on the code related to the addition and resolving of value path and constructor path! |
50f8e11
to
c702368
Compare
343afbe
to
a3c8465
Compare
a3c8465
to
586a79b
Compare
2bba7d1
to
6138d19
Compare
5a411b1
to
0272288
Compare
99ec79b
to
294d7bd
Compare
Here is the state of this PR: Jump from occurrence to implementationIt works quite well I think! Both inside a file, and "cross module". Jump from occurrence to documentationThis works well for cross-compilation-unit occurrences, but not intra-compilation unit. The way the "jump to documentation" links are display is temporary. Count occurrencesThere is a command to "count occurrences". It creates a "tree of occurrence information", where each identifier has a number of direct and indirect occurrences. A node is an identifier's information, the children are the information of all its "subidentifiers. There is also a command to "aggregate" occurrences, which takes several marshalled tables, unmarshall them, and aggregate them in one table which is then marshall. ConstructorsConstuctors are a bit harder to resolve (there can be several The testA good way of looking at the current state and its problems is by reading the test. ConclusionYou can play with the jump to occurrences and implementation here. |
2cdbecb
to
b7cf302
Compare
e884a33
to
9e3efb6
Compare
7053c25
to
0bc0fb0
Compare
I have removed the two things that I could not get right in this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive work :)
doc/driver.mld
Outdated
@@ -750,6 +760,7 @@ let compiled = compile_all () in | |||
let linked = link_all compiled in | |||
let () = index_generate () in | |||
let _ = js_index () in | |||
let _ = count_occurrences (Fpath.v "occurrences.txt") in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a text file. Any way to render it in a readable way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "print_occurrences" binary defined in the test allows to render it in a readable way. Do you think I should include in odoc
itself a way to render it as readable?
Also, maybe the count-occurrences
command should produce files with a "fixed" extension, e.g. *.occ-odoc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took inspiration from the constraints we give to the output name of a file representing a source tree.
So, the output file name of the count-occurrences
and aggregate-occurrences
must now have occurrences-
prefix and odoc
extension.
Fs.Directory.mkdir_p (Fs.File.dirname dst); | ||
let oc = open_out_bin (Fs.File.to_string dst) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for later: It seems that all Odoc commands create the parent directories of files they create. It would be nice to have this written down in the form of a factorized function.
(Ok []) input | ||
>>= fun files -> Ok (List.concat files) | ||
|
||
let aggregate files file_list ~warnings_options:_ ~dst = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the input and output files could be large. What do you think of using a sorted line-based format for both the inputs and the output ?
Sorted files can be merged in a streaming way and the command no longer needs to hold all the inputs and outputs in memory at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree that a data structure that would allow to merge several "occurrence tables" (even many of them) in O(n)
time (n
being the total number of elements) and O(t)
space (t
being the number of tables to merge) is possible as you suggest, interesting and a good thing to have, let's do that in a future PR!
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
- Handle `Pextra_ty` in `is_persistent` - alias `Tast_iterator.default_iterator` to improve readability - Remove possibility for `jump_to` type to have different types for doc and impl - Factorize all instances of `contains_double_underscore` - Use `ModuleName.is_hidden` instead of inlining its definition... - Handle `ClassType` occurrences in compile, and avoid future miss by having an exhaustive match. Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
- Remove now unused is_internal_rec. It was added for non-persistent values, but this has been delayed to another PR - Remove TODO comment on Classes when building the environment: classes cannot contain items that are contained in our current set of occurrence kinds (values, modules, module types, ...) Signed-off-by: Paul-Elliot <[email protected]>
Similar to source trees: must have `odoc` extension and `occurrences-` prefix Signed-off-by: Paul-Elliot <[email protected]>
They used to not be raised, as with non-persistent occurrences, some of them could never be got rid of! Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
This reverts commit 9eee684.
This reverts commit e38f414.
This reverts commit 5826eb1.
The resolving of constructor paths introduced was wrong and never tested. It is removed in this commit as well. Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
ead3969
to
c7b684a
Compare
Could we please avoid the change from Map to Hashtbl in Ident_env? The reasoning behind this is a change suggested by @lpw25 a while back to resolve the 'root' of a reference during the load phase so that references end up looking more like paths. The reasoning behind that is to have lexical scoping of references rather than the dynamic scoping we currently have - that way we can avoid having to make this sort of change. Switching to Hashtbls will make that much trickier. This is somewhat contingent on my guess that the change to ident_env is separable from the rest of the PR - if it isn't we might have to just commit this and solve the reference issue when we get around to it. |
Sure, I'll revert that. If I remember correctly, it is again part of the changes I made for non-persistent occurrences, that are not included anymore in this PR. I left it as it seemed an improvement, but that was not deeply though! |
2f8b0d3
to
e400590
Compare
This reverts commit 7e4e716.
Signed-off-by: Paul-Elliot <[email protected]>
Co-authored-by: Jules Aguillon <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now perfect :) Let's merge!
I agree, lovely stuff! We might want to revisit how and when we process stuff (see https://hackmd.io/__sMo7xRSEGDephPhtDpNQ for some thoughts), but this is all consistent with the current handling so no sense in delaying it. |
This PR is not fully finished, but I already open it to already leave the possibility to comment on the approach.
It adds to the source information the occurrences of each identifier.
This information can be used for at least two things:
List.fold_left
, ...)odoc
and all of its dependencies, we can find the number of occurrences for each identifier in this file, generated by the reference driver.Concretely, this PR adds:
--count-occurrences
flag to thecompile
command, which makesodoc
look for a.cmt
file and gather all occurrences in it (as odocPath
s) as source information. If the source rendering is also activated, this information will be usedcount-occurences
command, which Marshal aMap
from odocIdentifier.t
toint
, corresponding to the number of occurrences.What can be improved:
Add information of other kinds of nodes, such as constructors...Delayed to future PRHandle "local" identifier (they are currently not counted in the total, neither they have links to documentation in the rendered source code)Delayed to future PR