dblaikie added a comment. In D82617#2119394 <https://reviews.llvm.org/D82617#2119394>, @sammccall wrote:
> 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. Fair - I'd be willing to classify this particular case as pretty close to a false positive. But for me it'd be under the bucket of false positive's like GCC's -Wparetheses which warns on assert("message" && x || y) (GCC warns about the user probably meaning (x || y), Clang doesn't because it notices you get the same answer with or without the parens) - a bit annoying, but probably harmless enough to add the parens to let GCC users keep getting the warnings (& in that case - just means they'll be less likely to break LLVM self-hosting buildbots than if we disabled the GCC warning entirely) > 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. If there wasn't any name hiding, I wouldn't have a problem with the code as-is. For me it's about the call-sites rather than the implementation. Looking at a derived class never gives you the whole picture of the class's API anyway. > 3. The question of whether people like the concrete names or the overload set > chosen in ThreadsafeFS is irrelevant here. Yep - agreed. That's a conversation for other reviews/threads. > 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. Appreciate the summary/thoughts! 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
