This is an archive of the discontinued LLVM Phabricator instance.

[CFLAA] Code cleanup: group all graph-building codes into one class
ClosedPublic

Authored by grievejia on Jun 11 2016, 3:26 PM.

Details

Summary

This patch introduce no functionality changes.

It added a new class CFLGraphBuilder which groups together all codes related to graph building. This change effectively reduce the number of arguments for functions like "buildGraphFrom()", "addInstructionToGraph()", etc.

Diff Detail

Event Timeline

grievejia updated this revision to Diff 60448.Jun 11 2016, 3:26 PM
grievejia retitled this revision from to [CFLAA] Code cleanup: group all graph-building codes into one class.
grievejia updated this object.
grievejia added a subscriber: llvm-commits.

Thanks for the patch.

lib/Analysis/CFLAliasAnalysis.cpp
588

Redundant private

728

Things that look and feel like accessors (but that visibly modify the state of their object) seem sketchy to me.

849–850

Useless newline

853

I don't like that this builder is essentially partially-alive (Graph has been moved from, ReturnedValues hasn't) for the majority of this function.

Can we either:

  • Have get*() return references (that we move from at the end of the function), or
  • Move everything out of Builder at the top of the function, and declare it entirely dead+useless

instead?

grievejia updated this revision to Diff 60597.Jun 13 2016, 1:30 PM
grievejia edited edge metadata.
grievejia marked 3 inline comments as done.

Style fixes

grievejia marked an inline comment as done.Jun 13 2016, 1:31 PM

Looks good with one more comment.

lib/Analysis/CFLAliasAnalysis.cpp
728

...Half done?

If you want to keep the std::move in this accessor, that's fine -- maybe we can just rename it to something like takeReturnValues, or anything that makes it obvious that it's going to modify the object.

grievejia updated this revision to Diff 60639.Jun 13 2016, 6:48 PM

Rename getReturnValue() to takeReturnValue()

george.burgess.iv edited edge metadata.

LGTM. Thanks for the patch!

This revision is now accepted and ready to land.Jun 14 2016, 11:05 AM
This revision was automatically updated to reflect the committed changes.