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

Autoselect #29

Merged
merged 12 commits into from
Feb 28, 2019
Merged

Autoselect #29

merged 12 commits into from
Feb 28, 2019

Conversation

Jafosterja
Copy link

@Jafosterja Jafosterja commented Feb 26, 2019

Added basic implantation of automatic structure selection. This implementation will auto select structures full of entirely function pointers.

Closes #5
Closes #32

Nixoncole
Nixoncole previously approved these changes Feb 26, 2019
Copy link

@Nixoncole Nixoncole left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@tim-pugh tim-pugh left a comment

Choose a reason for hiding this comment

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

Reviewed code in Github, rebuilt and added James's changes to poc and tested with him. All looks good! Great Job!

Update: Some of the other pull requests that came in created a merge conflict I believe, sorry. Got to take back my approval.

};

class Randstruct : public RecordFieldReorganizer {
public:
//Automatic Structure selection

Choose a reason for hiding this comment

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

Since this method is public, we should use Doxygen syntax and give a better description here.

/// Determines if the Record can be safely and easily randomized based on certain criteria (see implementation).

Copy link

@connorkuehl connorkuehl left a comment

Choose a reason for hiding this comment

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

I know this wasn't written in as an acceptance criteria, but I think this PR should include the command line argument to enable automatic structure selection so that if it's included with our RFC then we are feature-complete.

@tim-pugh
Copy link
Member

I know this wasn't written in as an acceptance criteria, but I think this PR should include the command line argument to enable automatic structure selection so that if it's included with our RFC then we are feature-complete.

This was discussed in the meeting after you left but they're working on it Wed

Copy link
Member

@tim-pugh tim-pugh left a comment

Choose a reason for hiding this comment

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

@Jafosterja @jcantrell I opened a new issue to add the command line argument

This will be checked at our entry point, so we don't
need to check this here.
@connorkuehl connorkuehl requested review from connorkuehl and removed request for connorkuehl February 27, 2019 23:43
@connorkuehl
Copy link

PR looks feature-complete. The no_randomize_layout attribute is not in develop, so struct notautoselect from James's poc.c will be auto-selected.

Argument is used like:

clang -randstruct-auto poc.c -o poc

image

@connorkuehl connorkuehl dismissed their stale review February 28, 2019 00:10

Contributed code here

@Nixoncole Nixoncole self-requested a review February 28, 2019 00:10
Copy link

@Nixoncole Nixoncole left a comment

Choose a reason for hiding this comment

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

Small changes to aesthetics

@@ -1751,6 +1751,9 @@ def freroll_loops : Flag<["-"], "freroll-loops">, Group<f_Group>,
HelpText<"Turn on loop reroller">, Flags<[CC1Option]>;
def fno_reroll_loops : Flag<["-"], "fno-reroll-loops">, Group<f_Group>,
HelpText<"Turn off loop reroller">;
def randstruct_auto : Flag<["-"], "randstruct-auto">,
HelpText<"Enable automatic structure selection for field randomization; "
"disabled with no_randomize_layout">, Flags<[CC1Option]>;

Choose a reason for hiding this comment

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

Should we notate that this is disabled with the "attribute no_randomize_layout"?

Choose a reason for hiding this comment

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

I think that'd be wise. Certainly more clear than much of what exists already.

Copy link

@Nixoncole Nixoncole left a comment

Choose a reason for hiding this comment

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

Small changes to aesthetics

for (auto f : D->fields()){
//If an element of the structure does not have a
//function type is not a function pointer
if(f->getFunctionType() == nullptr){ return false; }

Choose a reason for hiding this comment

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

Double spaces here intentional?

Copy link

@jeffreytakahashi jeffreytakahashi left a comment

Choose a reason for hiding this comment

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

Other than the thoughts that Cole noted I think this is A+.

Copy link

@jeffreytakahashi jeffreytakahashi left a comment

Choose a reason for hiding this comment

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

Solid.

@tim-pugh tim-pugh merged commit 3a0479d into develop Feb 28, 2019
@connorkuehl connorkuehl deleted the Autoselect branch July 6, 2019 15:07
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.

5 participants