ckandeler added a comment.
================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76 ConstructorOrDestructor, + UserProvided, ---------------- sammccall wrote: > sammccall wrote: > > nridge wrote: > > > ckandeler wrote: > > > > ckandeler wrote: > > > > > sammccall wrote: > > > > > > sammccall wrote: > > > > > > > sammccall wrote: > > > > > > > > Can you give some background here or on the bug tracker about > > > > > > > > what kind of distinction you're trying to draw here and why > > > > > > > > it's important? > > > > > > > > (Most clients are never going to benefit from nonstandard > > > > > > > > modifiers so they should be pretty compelling) > > > > > > > as well as being jargony, "user-provided" has a specific > > > > > > > technical meaning that I don't think you intend here. For > > > > > > > example, `auto operator<=>(const S&) const = default` is not > > > > > > > user-provided (defaulted on first declaration). > > > > > > > https://eel.is/c++draft/dcl.fct.def.default#5 > > > > > > > > > > > > > > If we need this and can't get away with reusing `defaultLibrary` > > > > > > > (which would include `std::`) then maybe we should add something > > > > > > > like `builtin` which seems quite reusable. > > > > > > Since we often can't say whether an operator is user-provided or > > > > > > not (in templates), we should consider what we want the > > > > > > highlighting to be in these cases. > > > > > > (If templates should be highlighted as built-in, we should prefer a > > > > > > modifier like `UserProvided`, if they should be highlighted as > > > > > > user-provided, we should prefer a modifier like `Builtin`) > > > > > > as well as being jargony, "user-provided" has a specific technical > > > > > > meaning that I don't think you intend here. For example, `auto > > > > > > operator<=>(const S&) const = default` is not user-provided > > > > > > (defaulted on first declaration). > > > > > > https://eel.is/c++draft/dcl.fct.def.default#5 > > > > > > > > > > > > If we need this and can't get away with reusing `defaultLibrary` > > > > > > (which would include `std::`) then maybe we should add something > > > > > > like `builtin` which seems quite reusable. > > > > > > > > > > I use "userProvided" here in the sense of "not built-in" or > > > > > "overloaded". I do not cling to the term, and have also suggested the > > > > > opposite default of using "builtin" in another comment. > > > > > Since we often can't say whether an operator is user-provided or not > > > > > (in templates), we should consider what we want the highlighting to > > > > > be in these cases. > > > > > > > > True, I have not considered this. > > > > > > > > > (If templates should be highlighted as built-in, we should prefer a > > > > > modifier like `UserProvided`, if they should be highlighted as > > > > > user-provided, we should prefer a modifier like `Builtin`) > > > > > > > > Intuitively, it seems we should be conservative and not claim the > > > > operator is overloaded unless we know it is. So "built-in" might then > > > > mean "we can't prove it's not a built-in". It's probably not worth to > > > > introduce a modifier CouldBeEitherWay just to explicitly express > > > > ambiguity ;) > > > > Since we often can't say whether an operator is user-provided or not > > > > (in templates), we should consider what we want the highlighting to be > > > > in these cases. > > > > (If templates should be highlighted as built-in, we should prefer a > > > > modifier like `UserProvided`, if they should be highlighted as > > > > user-provided, we should prefer a modifier like `Builtin`) > > > > > > In my mind, "go-to-definition on this operator symbol will take me to a > > > function declaration/definition" is a good match for "I want this colored > > > differently". (Which would imply treating dependent operator calls where > > > we can't figure out an overloaded operator target even heuristically, as > > > "built-in".) > > > Can you give some background here or on the bug tracker about what kind > > > of distinction you're trying to draw here and why it's important? > > > (Most clients are never going to benefit from nonstandard modifiers so > > > they should be pretty compelling) > > > > This was one of the biggest questions I had about this patch - just hoping > > it doesn't get missed. > > Intuitively, it seems we should be conservative and not claim the operator > > is overloaded unless we know it is. > > This feels a bit circular, if we agree we're not going to introduce a > `CouldBeEitherWay` then why is "built-in" a more conservative claim than > "overloaded"? > > I'm inclined towards `builtin` as a modifier because I think for language > entities as a whole (types, functions etc, not just operators) it's the > exception. It also seems easier to name and define. > > > In my mind, "go-to-definition on this operator symbol will take me to a > > function declaration/definition" is a good match for "I want this colored > > differently". > > This mostly makes sense to me, but: > - I don't think we should actually run all the heuristics logic > - if there's probably a definition available but we can't resolve it due to > templates, I'd still like to know something's up > > I think my internal question is more like "is this a trivial arithmetic > shift, or something potentially complicated"? And I think depending on > template resolution is "potentially complicated". (Maybe trivial in the end, > but so might be an overloaded operator) > > Intuitively, it seems we should be conservative and not claim the operator > > is overloaded unless we know it is. > > This feels a bit circular, if we agree we're not going to introduce a > `CouldBeEitherWay` then why is "built-in" a more conservative claim than > "overloaded"? > > I'm inclined towards `builtin` as a modifier because I think for language > entities as a whole (types, functions etc, not just operators) it's the > exception. It also seems easier to name and define. > > > In my mind, "go-to-definition on this operator symbol will take me to a > > function declaration/definition" is a good match for "I want this colored > > differently". > > This mostly makes sense to me, but: > - I don't think we should actually run all the heuristics logic > - if there's probably a definition available but we can't resolve it due to > templates, I'd still like to know something's up > > I think my internal question is more like "is this a trivial arithmetic > shift, or something potentially complicated"? And I think depending on > template resolution is "potentially complicated". (Maybe trivial in the end, > but so might be an overloaded operator) The documentation for BinaryOperator says: /// Within a C++ template, whether BinaryOperator or CXXOperatorCallExpr is /// used to store an expression "x + y" depends on the subexpressions /// for x and y. If neither x or y is type-dependent, and the "+" /// operator resolves to a built-in operation, BinaryOperator will be /// used to express the computation (x and y may still be /// value-dependent). If either x or y is type-dependent, or if the /// "+" resolves to an overloaded operator, CXXOperatorCallExpr will /// be used to express the computation. With the patch as-is (possibly with the UserProvided/BuiltIn switch) , this should result exactly in what you want (if I understand you correctly). However, it does not match my observation: I always see the normal operator types. Am I missing something or is the documentation wrong? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136594/new/ https://reviews.llvm.org/D136594 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
