This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Move ImportedFunctionsInliningStatistics to Analysis
ClosedPublic

Authored by mtrofin on Jan 20 2021, 12:41 PM.

Details

Summary

This is related to D94982. We want to call these APIs from the Analysis
component, so we can't leave them under Transforms.

Diff Detail

Event Timeline

mtrofin created this revision.Jan 20 2021, 12:41 PM
mtrofin requested review of this revision.Jan 20 2021, 12:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2021, 12:41 PM
mtrofin added inline comments.Jan 20 2021, 12:42 PM
llvm/lib/Analysis/ImportedFunctionsInliningStatistics.cpp
189
NOTE: this was the linter having initiative - I'd leave the change in, albeit unrelated.
aeubanks accepted this revision.Jan 20 2021, 12:54 PM
This revision is now accepted and ready to land.Jan 20 2021, 12:54 PM
This revision was landed with ongoing or failed builds.Jan 20 2021, 1:18 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.EditedJan 26 2021, 9:21 AM

In your commit the message does not include Reviewed by:. Many people agree that both Reviewed by: & Differential Revision: should be present. The Reviewed by: list indicates people who acknowledged the patch. (The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless.)

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/utils/git/pre-push.py to prevent accidental Summary:, Reviewers:, Subscribers: and Tags: in the presence of Differential Revision:.

In your commit the message does not include Reviewed by:. Many people agree that both Reviewed by: & Differential Revision: should be present. The Reviewed by: list indicates people who acknowledged the patch. (The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless.)

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/utils/git/pre-push.py to prevent accidental Summary:, Reviewers:, Subscribers: and Tags: in the presence of Differential Revision:.

FWIW, my take on it would be that including "Reviewed by:" is not harmful/may be useful (as opposed to the other fields (except Differential Revision), which I think actively add noise to the commit messages), but I wouldn't say it's required/don't think it rises to the level of needing to be fixed/corrected.