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

Adds duplicate feature #375

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Adds duplicate feature #375

wants to merge 7 commits into from

Conversation

aaronjeline
Copy link
Collaborator

Adds a variable duplication functionality

Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

I don't understand what's going on here. Should this PR maybe be a draft? Before review, I'd expect some remark about why this functionality is being added unused and untested.

clang/include/clang/3C/Duplicate.h Outdated Show resolved Hide resolved
clang/include/clang/3C/Duplicate.h Outdated Show resolved Hide resolved
Aaron Eline and others added 2 commits January 13, 2021 11:15
Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>
Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>
// or None if no variable matching P is found
Option<std::string> createDuplicate(clang::ASTContext &Context,
clang::Rewriter &R, ProgramInfo &Info,
clang::Decl *ToScan, selector P);
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering: Why this interface for determining the variable to duplicate? That is, I could imagine just specifying a single Decl and duplicating that. Or I could imagine providing a variable name, and you'd search for the proper decl. Just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This just seemed like the most general interface in case this becomes useful for other modules down the line. One change could be adding some top level helpers for simpler functionality.

bool VisitVarDecl(VarDecl *VD) {
// If the selctor matches execute the duplicate function
// and store the new name in the optional
if(P(VD)){
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this is true twice? I imagine you expect it won't be, but is there a reason it couldn't?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The return false at the bottom should prevent this. It will stop traversing once it hits a positive

Copy link
Member

@mwhicks1 mwhicks1 left a comment

Choose a reason for hiding this comment

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

Looks fine, but did leave one comment that might identify a problem.

Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

Once again: are we merging this unused and untested??

clang/lib/3C/Duplicate.cpp Outdated Show resolved Hide resolved
Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>
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