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

Freshen levee package docs #220

Merged
merged 3 commits into from
Dec 14, 2020

Conversation

mlevesquedion
Copy link
Contributor

This PR aims to improve the documentation for the levee and propagation packages. Specifically, it adds a few documentation comments and removes/rectifies a few incorrect pieces of documentation.

@@ -51,9 +57,6 @@ func Dfs(n ssa.Node, conf source.Classifier, taggedFields fieldtags.ResultType)
return record
}

// dfs performs Depth-First-Search on the def-use graph of the input Source.
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this comment? I think the second part of the comment was very useful for a newb like me. Did the behavior mentioned in the comment change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last sentence is actually incorrect. When the traversal encounters a Sanitizer, it merely records that the sanitizer has been reached. Sanitizers are not otherwise treated differently from other calls.

Removing the comment is the wrong thing to do here, though. I have rewritten the comment instead, thank you for your input! WDYT about this new comment?

Copy link
Member

Choose a reason for hiding this comment

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

LGTM! Thanks for adding great documentation to this method.

@@ -51,9 +57,6 @@ func Dfs(n ssa.Node, conf source.Classifier, taggedFields fieldtags.ResultType)
return record
}

// dfs performs Depth-First-Search on the def-use graph of the input Source.
Copy link
Member

Choose a reason for hiding this comment

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

LGTM! Thanks for adding great documentation to this method.

Comment on lines 43 to 44
// Dfs performs a depth-first search of the graph formed by SSA Referrers and
// Operands relationships.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding beginning at the root node n to the doc comment to distinguish this exported Dfs as the entrypoint for a given root vs the recursive step dfs below.

Copy link
Contributor Author

@mlevesquedion mlevesquedion Dec 14, 2020

Choose a reason for hiding this comment

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

Done. Thanks!

@mlevesquedion mlevesquedion merged commit e2f89b6 into google:master Dec 14, 2020
@mlevesquedion mlevesquedion deleted the freshen-levee-docs branch December 14, 2020 18:56
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.

None yet

3 participants