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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
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#>
+}
----------------
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?




https://reviews.llvm.org/D38538