sammccall abandoned this revision. sammccall added a comment. Abandoning, we'll do this in clangd or find an acceptable way to silence it (see D82736 <https://reviews.llvm.org/D82736>).
In D82617#2119144 <https://reviews.llvm.org/D82617#2119144>, @dblaikie wrote: > In D82617#2119138 <https://reviews.llvm.org/D82617#2119138>, @sammccall wrote: > > > > Guess perhaps a different question: Why don't you want this for clangd? > > > Does it make the codebase better by not adhering to this particular > > > warning? > > > > Yes, exactly. (Sorry if this wasn't explicit). > > > Sorry - poor phrasing on my part. Seems we disagree on this - I think it's > probably a good thing to adhere to, you don't. Yeah, I think we disagree, and that's OK! We need to stop emitting a warning, and apart from that, this doesn't seem like a terribly important question that needs an LLVM-wide policy. On stylistic questions like this, I think code owners generally decide, right? So my intent was to drop this proposed change for clang (no consensus) and take these opinions under advisement for clangd. I didn't re-engage here on the substance as I think all the arguments are played out and nobody's mind is being changed, and I figured it was time to move on. > I'd like to better understand the difference of opinions. Sure, and apologies I didn't read it that way. I can summarize the counterargument as: 1. I agree that this warning flags cases with confusing code, though in some cases (like ThreadsafeFS::view) I think the potential for confusion *due to the name-hiding + overloading pattern* is very low. 2. Marking one method of an overload set virtual and implementing others in terms of it is the most direct way to express what we're trying to do here. (e.g. in Java which has no equivalent name-hiding pattern, nobody really takes issue with this). Therefore if the name-hiding is harmless or close-to-it, it's the clearest expression of this pattern. 3. The question of whether people like the concrete names or the overload set chosen in ThreadsafeFS is irrelevant here. I think there's a fair amount of disagreement about (1), nobody really presented a counterargument to (2), and for some reason we spent a lot of time on (3) which seems to be a total bikeshed on a tangentially related topic that had been settled in the usual way. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82617/new/ https://reviews.llvm.org/D82617 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
