[PATCH] D38538: Avoid printing some redundant name qualifiers in completion

[Alex Lorenz via Phabricator via cfe-commits]

arphaman accepted this revision.
arphaman added inline comments.
This revision is now accepted and ready to land.


================
Comment at: test/CodeCompletion/qualifiers-as-written.cpp:29
+  // CHECK-2: COMPLETION: func : [#int#]func(<#foo a#>, <#bar b#>, <#ns::bar c#>, <#ns::baz d#>
+  // CHECK-2: COMPLETION: func : [#int#]func(<#foo::type a#>, <#bar b#>, <#baz c#>
+}
----------------
ilya-biryukov wrote:
> arphaman wrote:
> > Sorry, I see the issue now. However, I don't think that we'd like to change the signature for a function like this, as we'd still prefer to show `func (foo::type, ns::bar, ns::baz);` on this line.
> > 
> > In Xcode we actually avoid the problem with `std::vector<>`s that you've pointed out entirely by using `value_type`. I'll check what our solution does.
> > 
> > Btw, maybe using things like `value_type` is completely wrong (with or without the qualifier)? If we have `std::vector<int>` shouldn't we rather show `push_back(int _Value)`, rather than the `value_type`? Perhaps this kind of change should be discussed with a wider community somehow to find out what's best for all users.
> Using `int _Value` may be good in case of `vector<int>`, but it would probably loose typedefs, i.e. `vector<int32>` would still have `int`, which may be undesirable.
> Also, for `vector<vector<int>>`, the type inside `push_back` would probably end up being `vector<int, std::allocator<int>>`. 
> 
> 
> As for the current vs new behavior, I can totally see why you want to see more context in completion items. 
> 
> I would argue that the current code does not do a great job there, as it only adds qualifiers to unqualified references. But the context may be missing for qualified references as well.
> For example, in the following code:
> ```
>   template <class T, class Alloc = std::allocator<T>>
>   struct vector {
>     typedef T value_type;
> 
>     typename value_type::some_type foo();
>     value_type bar()
>   };
> ```
> 
> Completion item for `vector<X>::bar` will have return type `vector<X, std::allocator<X>>::value_type`. However, completion item for `vector<X, std::allocator<X>>::foo` will have return type `value_type::some_type` (note that no `vector<X, std::allocator<X>>` qualifier was added).
> 
> I would also question the intent of the current implementation. The following line suggest that adding those qualifers was not intended in the first place:
> ```
>   Policy.SuppressUnwrittenScope = true;
> ```
> But that's just a wild guess, I may be completely wrong.
> 
> 
> That being said, I don't think there is one right preference, different people may prefer different options. We can surely discuss that in a wider community, though.
> 
> Would you be open to adding an option for that in completion and keeping the current behavior as a default?
> 
> 
Sorry about the delay.

I clarified Xcode's behavior - we actually didn't show the qualifiers for typedefs and typealiases because of an older Clang, with ToT the behavior is as you describe.

So this patch will remove qualifiers for structs and classes, like in the `func` example here. I'm willing to accept it right now. We might have to revisit this change at some point though. Let's also keep one behavior right now, I don't think we need to diverge yet.



https://reviews.llvm.org/D38538