sammccall added a comment. For my part, I still need to understand why we want the `builtin`/`UserModified` modifier. (The `operator` highlight kind seems obvious to me).
From earlier comments: >> 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. ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76 ConstructorOrDestructor, + UserProvided, ---------------- ckandeler wrote: > sammccall wrote: > > ckandeler wrote: > > > 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? > > Can you reduce this to an example where `clang -fsyntax-only -Xclang > > -ast-dump` doesn't print what you expect? > > > > (if this isn't possible, then it might be a bug in the patch) > $ cat test.cpp > template<typename T> class MyTemplate { > void run(typename T::A t1, typename T::A t2) { return t1 == t2; } > } > $ clang -fsyntax-only -Xclang -ast-dump test.cpp > > TranslationUnitDecl 0x55986dab0dd8 <<invalid sloc>> <invalid sloc> > |-TypedefDecl 0x55986dab1640 <<invalid sloc>> <invalid sloc> implicit > __int128_t '__int128' > | `-BuiltinType 0x55986dab13a0 '__int128' > |-TypedefDecl 0x55986dab16b0 <<invalid sloc>> <invalid sloc> implicit > __uint128_t 'unsigned __int128' > | `-BuiltinType 0x55986dab13c0 'unsigned __int128' > |-TypedefDecl 0x55986dab1a28 <<invalid sloc>> <invalid sloc> implicit > __NSConstantString '__NSConstantString_tag' > | `-RecordType 0x55986dab17a0 '__NSConstantString_tag' > | `-CXXRecord 0x55986dab1708 '__NSConstantString_tag' > |-TypedefDecl 0x55986dab1ac0 <<invalid sloc>> <invalid sloc> implicit > __builtin_ms_va_list 'char *' > | `-PointerType 0x55986dab1a80 'char *' > | `-BuiltinType 0x55986dab0e80 'char' > |-TypedefDecl 0x55986daf65a8 <<invalid sloc>> <invalid sloc> implicit > __builtin_va_list '__va_list_tag[1]' > | `-ConstantArrayType 0x55986daf6550 '__va_list_tag[1]' 1 > | `-RecordType 0x55986dab1bb0 '__va_list_tag' > | `-CXXRecord 0x55986dab1b18 '__va_list_tag' > `-ClassTemplateDecl 0x55986daf6750 <test.cpp:1:1, line:3:1> line:1:28 > MyTemplate > |-TemplateTypeParmDecl 0x55986daf6600 <col:10, col:19> col:19 typename > depth 0 index 0 T > `-CXXRecordDecl 0x55986daf66c0 <col:22, line:3:1> line:1:28 class > MyTemplate definition > |-DefinitionData empty aggregate standard_layout trivially_copyable pod > trivial literal has_constexpr_non_copy_move_ctor can_const_default_init > | |-DefaultConstructor exists trivial constexpr needs_implicit > defaulted_is_constexpr > | |-CopyConstructor simple trivial has_const_param needs_implicit > implicit_has_const_param > | |-MoveConstructor exists simple trivial needs_implicit > | |-CopyAssignment simple trivial has_const_param needs_implicit > implicit_has_const_param > | |-MoveAssignment exists simple trivial needs_implicit > | `-Destructor simple irrelevant trivial needs_implicit > |-CXXRecordDecl 0x55986daf6990 <col:22, col:28> col:28 implicit class > MyTemplate > `-CXXMethodDecl 0x55986daf6d18 <line:2:5, col:73> col:10 compare 'bool > (typename T::A, typename T::A)' > |-ParmVarDecl 0x55986daf6b10 <col:18, col:32> col:32 referenced t1 > 'typename T::A' > |-ParmVarDecl 0x55986daf6bd0 <col:36, col:50> col:50 referenced t2 > 'typename T::A' > `-CompoundStmt 0x55986daf6e50 <col:54, col:73> > `-ReturnStmt 0x55986daf6e40 <col:56, col:69> > `-BinaryOperator 0x55986daf6e20 <col:63, col:69> '<dependent type>' > '==' > |-DeclRefExpr 0x55986daf6de0 <col:63> 'typename T::A' lvalue > ParmVar 0x55986daf6b10 't1' 'typename T::A' > `-DeclRefExpr 0x55986daf6e00 <col:69> 'typename T::A' lvalue > ParmVar 0x55986daf6bd0 't2' 'typename T::A' > > > The documentation for BinaryOperator says: It indeed looks like the documentation is wrong :-( Based on that, your implementation looks right to me. 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
