This is an archive of the discontinued LLVM Phabricator instance.

Revert "[NFC] factor out unrolling decision logic"
ClosedPublic

Authored by GMNGeoffrey on Aug 18 2021, 11:38 AM.

Details

Summary

This patch added a requirement for C++17, while LLVM is supposed to
build with C++14
(https://llvm.org/docs/CodingStandards.html#c-standard-versions). Posted
a note to the original review thread (https://reviews.llvm.org/D106001).

This reverts commit 4d559837e887c278d7c27274f4f6b1b78b97c00d.

Diff Detail

Event Timeline

GMNGeoffrey created this revision.Aug 18 2021, 11:38 AM
GMNGeoffrey requested review of this revision.Aug 18 2021, 11:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2021, 11:38 AM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 18 2021, 11:39 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

What is the point of creating a review if you are going to commit it immediately?
Can you not just push the change directly?
This only creates noise.

What is the point of creating a review if you are going to commit it immediately?
Can you not just push the change directly?
This only creates noise.

Hi Roman. I have heard and read unclear and conflicting guidance on usage of phabricator, llvm-commits, git, and arc for LLVM and how all these relate to patches intended for pre- or post-commit review. To me, having everything on phab makes sense. Additionally, use of arc requires creating a diff first, I believe and helps with the fetch+rebase+push flow, which often needs to be repeated if something else was pushed in the meantime. In cases where pre-merge builds would be useful, but review would not, creating a phab diff has the additional advantage of triggering those. Finally, following a consistent workflow helps avoid errors in constructing git commands. For these reasons, I decided that the simplest thing to do was to make my workflow the same in all cases: arc diff + arc land. I apologize if that has created additional noise for you and I am open to further suggestions, assuming there is something approaching a community consensus on how these things should be done. IMO the LLVM review and development process could use quite a bit of simplification and modernization...