[PATCH] D83088: Introduce CfgTraits abstraction

Tue Oct 27 06:05:54 PDT 2020

rengolin added a comment.

I have read all of the comments in this review and have looked at all the other pending reviews because of this and I still see strong disagreement on the design and implementation.

Unfortunately, I can't contribute with the technical discussion because there's a lot more context and content here than I can absorb in the time I have available, but overall I think David's critics are very pertinent. They don't mean the code is wrong or bad, just that they are important questions that need answers. Some of the questions were answered, others weren't. This patch should not have been committed before those things were sorted out, as is clearly stated in the (existing) review policy (https://llvm.org/docs/CodeReview.html#acknowledge-all-reviewer-feedback).

I do appreciate that the other patches are "waiting" for this one and that it has been months, but this patch fundamentally changes the algorithm with a motivation that still isn't clear for two reasons:

1. It was initially stated that the motivation is to reduce the number of templates because the author doesn't like them, which is not a good reason.
2. Despite recurrent requests to show concrete code examples comparing current and new design, showing why it would be "easier to use", none have materialised (other than David's own attempts).

All of the other patches were approved by the same set of people. This is definitely not uncommon, but is highly susceptible to unconscious bias. Once David questioned the approach with concrete questions, concrete answers should address all of the points.

This patch should have never been committed in the first place, even with one approval.

In D83088#2342760 <https://reviews.llvm.org/D83088#2342760>, @dblaikie wrote:

> Sorry about the delays in review - but please revert this patch until we can hash out a few more details. I really don't think this is the best direction forward for a core abstraction & I'll do my best to explain why & try to understand where you're coming from.

The (current) review policy (https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed) is already clear enough:

"There is a strong expectation that authors respond promptly to post-commit feedback and address it. Failure to do so is cause for the patch to be reverted."

It's pretty clear that the paragraph applies to David's post-commit review.

> I'd really like to see examples like this ^ using the different abstractions under consideration here (classic GraphTraits, CfgTraits dynamic and static typed, perhaps what a static API would look like if it wasn't trying to be dynamic API compatible).

This is the main request that was left unaddressed and the one that has the highest impact on the design of the API as well as all the following patches. The fact that they have all been approved doesn't mean this one must, too.

Their own approval just means those changes look good with the current interpretation, not that they must land. It's entirely possible that they continue to be good after the API has changed (if it has), in which case the approvals can just be restated. But they may well disappear or change completely due to the change in API, and will need new reviews to avoid having an already approved review change substantially in content.

> Indeed template code can get a bit weird - but I'm not sure it's quite enough to justify the change in/complications of mulitple (static and dynamic) abstractions here just yet. It might be that taking a more structured C++ idiomatic approach to template design (like C++ standard container concept abstractions) might lead to more usable code without /some/ complexities (though may trade others).

And this is the main critic to the overall design choice, which I also agree completely. I'm not a big fan of complex template code myself, but the idioms are well known and they do simplify usage in many cases.

LLVM has had its fair share of discussions on the topics and we have developed multiple APIs and containers tho overcome deficiencies in the standard library, some of those concepts made into the standard. Some of those I have learned to like when I tried to implement otherwise and failed.

Only with concrete comparison of usage and patterns that we can make an informed decision and this is required to change a core concept of the compiler more than any other place. A single example where your pattern fits isn't enough to demonstrate that it will be generic and usable for all the other patterns that it may be used.

I'm sorry this delays more your work, but this is what working on an open source very large project entails. In comparison, LLVM is very fast compared to other OSS projects out there.

Also, echoing other people in this thread, this is more a case for an RFC thread on the list than code review. If the original thread didn't catch the attention of a public wide enough, ping the thread, talk to people on IRC or any other tool that works for you. We need consensus and we clearly don't have it here.

As an LLVM developer, you're expected to read the policy documents and follow the expectations, but not necessarily to interpret them in the same way we did when we wrote them. If they're confusing or imprecise, remember we're not writers, nor we're all native English speakers, nor we all have the same culture. Trying to clarify what they mean (as Sean and Mehdi tried to do) is the only way forward.

Please, revert this commit and discuss the merits of your approach on the list.

Thank you,
--renato

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83088/new/

https://reviews.llvm.org/D83088