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 Doxygen documentation for ResolveReferences #613

Merged
merged 6 commits into from
May 12, 2017
Merged

Add Doxygen documentation for ResolveReferences #613

merged 6 commits into from
May 12, 2017

Conversation

jnfoster
Copy link
Contributor

Also, refactor so that all booleans used to control forward resolution
are oriented in the same direction.

@@ -252,9 +251,8 @@ void ResolveReferences::end_apply(const IR::Node* node) {
refMap->updateMap(node);
}

/////////////////// visitor methods ////////////////////////
/****** Visitor methods ******/
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be a doxygen comment.

class ReferenceMap final : public ProgramMap, public NameGenerator {
bool isv1; // if true this is a map for a P4 v1.0 program (P4-14)
// Maps each path in the program to the corresponding declaration
/// If `isv1` is true, then map is for a P4_14 program
Copy link
Contributor

Choose a reason for hiding this comment

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

Maps for v1 programs are different because v1 programs allow declarations to come after uses.
This difference is carried even in P4-16 program that are generated from P4-14 programs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

std::set<const IR::IDeclaration*> used;

/// Map from `This` to declarations
/// @todo: briefly explain why different than `pathToDeclaration`
Copy link
Contributor

Choose a reason for hiding this comment

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

Because Path is a different class than This.
This is an experimental feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Added a note.

@@ -440,8 +438,10 @@ void ResolveReferences::postorder(const IR::Declaration_Instance *decl) {
}

bool ResolveReferences::preorder(const IR::Property *prop) {
/// @todo: why is `dynamic_cast` used here instead of `to`?
Copy link
Contributor

Choose a reason for hiding this comment

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

@ChrisDodd wrote this line, there are really no Attributes in P4-16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave the TODO for now.

enum class ResolutionType {
Any,
Type,
TypeVariable
};

/**
* Data structure used while resolving references that comprises:
* - A stack of namespaces
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the description of the fields belongs to the fields, not to the class.
This class is logically a stack of nested namespaces where the resolution of a symbol is attempted, inside-out.

bool anyOrder;

/// Indicates if it should be an error for one declaration to shadow another.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it indicates whether warnings should be emitted for shadowing.
It is never an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

bool checkShadow;

private:
/// Add @p ns to `context`
Copy link
Contributor

Choose a reason for hiding this comment

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

These next three comments are not very useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but worth having for uniformity IMO.

void addToGlobals(const IR::INamespace* ns);

/// Resolve @p path; @p isType indicates if a type is expected.
Copy link
Contributor

Choose a reason for hiding this comment

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

If 'isType' is true then the resolution will only return IR::Type nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

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.

3 participants