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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to