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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
