[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.

[Carlos Alberto Enciso via Phabricator via cfe-commits]

CarlosAlbertoEnciso added a comment.

In https://reviews.llvm.org/D46190#1135688, @rsmith wrote:

> The right way to handle this is to pass both the ultimately-selected declaration and the declaration found by name lookup into the calls to `MarkAnyDeclReferenced` and friends. We should mark the selected declaration as `Used` (when appropriate), and mark the found declaration as `Referenced`.
>
> We should not be trying to reconstruct which using declarations are in scope after the fact. Only declarations actually found by name lookup and then selected by the context performing the lookup should be marked referenced. (There may be cases where name lookup discards equivalent lookup results that are not redeclarations of one another, though, and to handle such cases properly, you may need to teach name lookup to track a list of found declarations rather than only one.)


Following your comments, I have replaced the scope traversal with `LookupName` in order to find the declarations.

But there are 2 specific cases:

- using-directives

The `LookupName` function does not record the using-directives. With this patch, there is an extra option to store those directives in the lookup results, to be processed during the setting of the 'Referenced' bit.

- namespace-alias

I am aware of your comment on the function that recursively traverse the namespace alias, but I can't see another way to do it. If you have a more specific idea, I am happy to explore it.

For both cases, may be `LookupName` function can have that functionality and store in the results any namespace-directives and namespace-alias associated with the given declaration.



================
Comment at: lib/Sema/Sema.cpp:1879
+void Sema::MarkNamespaceAliasReferenced(NamedDecl *ND) {
+  if (ND && !ND->isReferenced()) {
+    NamespaceAliasDecl *NA = nullptr;
----------------
probinson wrote:
> You could do this as an early return and reduce indentation in the rest of the method.
> ```
> if (!ND || ND->isReferenced())
>   return;
> ```
> 
Corrected to

```
if (!ND || ND->isReferenced())
  return;
```



================
Comment at: lib/Sema/Sema.cpp:1880
+  if (ND && !ND->isReferenced()) {
+    NamespaceAliasDecl *NA = nullptr;
+    while ((NA = dyn_cast<NamespaceAliasDecl>(ND)) && !NA->isReferenced()) {
----------------
probinson wrote:
> Initializing this to nullptr is redundant, as you set NA in the while-loop expression.
Removed the `nullptr` initialization.


================
Comment at: lib/Sema/Sema.cpp:1891
+/// \brief Mark as referenced any 'using declaration' that have introduced
+/// the given declaration in the current context.
+void Sema::MarkUsingReferenced(Decl *D, CXXScopeSpec *SS, Expr *E) {
----------------
probinson wrote:
> `\brief` is unnecessary, as we have auto-brief turned on.
Removed the `\brief` format.



================
Comment at: lib/Sema/Sema.cpp:1903
+  if (auto *NNS = SS ? SS->getScopeRep()
+                     : E ? dyn_cast<DeclRefExpr>(E)->getQualifier()
+                         : nullptr) {
----------------
probinson wrote:
> This dyn_cast<> can be simply a cast<>.
Changed the dyn_cast<> to cast<>


================
Comment at: lib/Sema/Sema.cpp:1916
+      if ((USD = dyn_cast<UsingShadowDecl>(DR)) && !USD->isReferenced()) {
+        if (USD->getTargetDecl() == D) {
+          USD->setReferenced();
----------------
probinson wrote:
> You could sink the declaration of USD like so:
> ```
> for (auto *DR : S->decls())
>   if (auto *USD = dyn_cast<UsingShadowDecl>(DR))
>     if (!USD->isReferenced() && USD->getTargetDecl() == D) {
> ```
> Also I would put braces around the 'for' loop body, even though it is technically one statement.
Not available in the new patch.


================
Comment at: lib/Sema/Sema.cpp:1927
+    // Check if the declaration was introduced by a 'using-directive'.
+    auto *Target = dyn_cast<NamespaceDecl>(DC);
+    for (auto *UD : S->using_directives())
----------------
probinson wrote:
> You verified that his is a namespace earlier, so use cast<> not dyn_cast<>.
Not available in the new patch.


https://reviews.llvm.org/D46190