[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
CarlosAlbertoEnciso added a comment. In https://reviews.llvm.org/D46190#1135295, @probinson wrote: > Style comments. > The two new Sema methods (for namespaces and using references) are C++ > specific, so SemaDeclCXX.cpp would seem like a more appropriate home for them. Both functions have been moved to SemaDeclCXX.cpp. https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
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(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(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(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(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(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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
CarlosAlbertoEnciso added a comment. rsmith Thanks very much for your review. I will address them. https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
CarlosAlbertoEnciso added a comment. @probinson Thanks very much for your review. I will address them. https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
rsmith added a comment.
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.)
Comment at: lib/Sema/Sema.cpp:1884-1885
+ ND = NA->getAliasedNamespace();
+ if (auto *NNS = NA->getQualifier())
+MarkNamespaceAliasReferenced(NNS->getAsNamespaceAlias());
+}
The loop and recursion here -- and indeed this whole function -- seem
unnecessary.
```
namespace A {} // #1
namespace X {
namespace B = A; // #2
}
namespace Y = X; // #3
namespace C = Y::B; // #4
```
Declaration `#4` should mark `#2` and `#3` referenced. So the only
'unreferenced namespace alias' warning we should get here is for `#4` -- and
there is no need for marking `#4` as referenced to affect `#2` or `#3`.
Comment at: lib/Sema/Sema.cpp:1890-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) {
Why is this ever appropriate? I would think we should just mark the named
declaration from the relevant lookup as referenced in all cases.
https://reviews.llvm.org/D46190
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
probinson added a comment.
Style comments.
The two new Sema methods (for namespaces and using references) are C++
specific, so SemaDeclCXX.cpp would seem like a more appropriate home for them.
Comment at: lib/Sema/Sema.cpp:1879
+void Sema::MarkNamespaceAliasReferenced(NamedDecl *ND) {
+ if (ND && !ND->isReferenced()) {
+NamespaceAliasDecl *NA = nullptr;
You could do this as an early return and reduce indentation in the rest of the
method.
```
if (!ND || ND->isReferenced())
return;
```
Comment at: lib/Sema/Sema.cpp:1880
+ if (ND && !ND->isReferenced()) {
+NamespaceAliasDecl *NA = nullptr;
+while ((NA = dyn_cast(ND)) && !NA->isReferenced()) {
Initializing this to nullptr is redundant, as you set NA in the while-loop
expression.
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) {
`\brief` is unnecessary, as we have auto-brief turned on.
Comment at: lib/Sema/Sema.cpp:1903
+ if (auto *NNS = SS ? SS->getScopeRep()
+ : E ? dyn_cast(E)->getQualifier()
+ : nullptr) {
This dyn_cast<> can be simply a cast<>.
Comment at: lib/Sema/Sema.cpp:1916
+ if ((USD = dyn_cast(DR)) && !USD->isReferenced()) {
+if (USD->getTargetDecl() == D) {
+ USD->setReferenced();
You could sink the declaration of USD like so:
```
for (auto *DR : S->decls())
if (auto *USD = dyn_cast(DR))
if (!USD->isReferenced() && USD->getTargetDecl() == D) {
```
Also I would put braces around the 'for' loop body, even though it is
technically one statement.
Comment at: lib/Sema/Sema.cpp:1927
+// Check if the declaration was introduced by a 'using-directive'.
+auto *Target = dyn_cast(DC);
+for (auto *UD : S->using_directives())
You verified that his is a namespace earlier, so use cast<> not dyn_cast<>.
https://reviews.llvm.org/D46190
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
probinson added a comment. @dblaikie is @rsmith back from the standards meeting yet? I hate to be a pest but this is blocking other work Carlos has in progress. https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
CarlosAlbertoEnciso added a comment. Ping. The review https://reviews.llvm.org/D44826 is already approved and it is dependent on this patch being reviewed. @rsmith Is there anything I can add to this patch? Thanks https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
probinson added a comment. @rsmith anything else needed here? https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
CarlosAlbertoEnciso added a comment. Ping. The review https://reviews.llvm.org/D44826 is already approved and it is dependent on this patch being reviewed. Is there anything I can add to this patch? Thanks very much. https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
CarlosAlbertoEnciso added a comment. Ping. Thanks https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
CarlosAlbertoEnciso added a comment. Ping. Thanks https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.
CarlosAlbertoEnciso updated this revision to Diff 147050.
CarlosAlbertoEnciso edited the summary of this revision.
CarlosAlbertoEnciso added a comment.
Address the issues raised by the reviewer (rsmith).
https://reviews.llvm.org/D46190
Files:
include/clang/Sema/Sema.h
lib/Sema/Sema.cpp
lib/Sema/SemaCXXScopeSpec.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaTemplate.cpp
test/PCH/cxx-templates.cpp
test/SemaCXX/referenced_alias_declaration_1.cpp
test/SemaCXX/referenced_alias_declaration_2.cpp
test/SemaCXX/referenced_using_all.cpp
test/SemaCXX/referenced_using_declaration_1.cpp
test/SemaCXX/referenced_using_declaration_2.cpp
test/SemaCXX/referenced_using_directive.cpp
Index: test/SemaCXX/referenced_using_directive.cpp
===
--- test/SemaCXX/referenced_using_directive.cpp
+++ test/SemaCXX/referenced_using_directive.cpp
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -ast-dump %s | FileCheck %s --strict-whitespace
+
+namespace N {
+ typedef int Integer;
+ int var;
+}
+
+void Fa() {
+ using namespace N; // Referenced
+ var = 1;
+}
+
+void Fb() {
+ using namespace N;
+ N::var = 1;
+}
+
+void Fc() {
+ using namespace N; // Referenced
+ Integer var = 1;
+}
+
+void Fd() {
+ using namespace N;
+ N::Integer var = 1;
+}
+
+//CHECK: |-FunctionDecl {{.*}} Fa 'void ()'
+//CHECK-NEXT: | `-CompoundStmt {{.*}}
+//CHECK-NEXT: | |-DeclStmt {{.*}}
+//CHECK-NEXT: | | `-UsingDirectiveDecl {{.*}} referenced Namespace {{.*}} 'N'
+//CHECK-NEXT: | `-BinaryOperator {{.*}} 'int' lvalue '='
+//CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'var' 'int'
+//CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+//CHECK-NEXT: |-FunctionDecl {{.*}} Fb 'void ()'
+//CHECK-NEXT: | `-CompoundStmt {{.*}}
+//CHECK-NEXT: | |-DeclStmt {{.*}}
+//CHECK-NEXT: | | `-UsingDirectiveDecl {{.*}} Namespace {{.*}} 'N'
+//CHECK-NEXT: | `-BinaryOperator {{.*}} 'int' lvalue '='
+//CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'var' 'int'
+//CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+//CHECK-NEXT: |-FunctionDecl {{.*}} Fc 'void ()'
+//CHECK-NEXT: | `-CompoundStmt {{.*}}
+//CHECK-NEXT: | |-DeclStmt {{.*}}
+//CHECK-NEXT: | | `-UsingDirectiveDecl {{.*}} referenced Namespace {{.*}} 'N'
+//CHECK-NEXT: | `-DeclStmt {{.*}}
+//CHECK-NEXT: | `-VarDecl {{.*}} var 'N::Integer':'int' cinit
+//CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+//CHECK-NEXT: `-FunctionDecl {{.*}} Fd 'void ()'
+//CHECK-NEXT: `-CompoundStmt {{.*}}
+//CHECK-NEXT: |-DeclStmt {{.*}}
+//CHECK-NEXT: | `-UsingDirectiveDecl {{.*}} Namespace {{.*}} 'N'
+//CHECK-NEXT: `-DeclStmt {{.*}}
+//CHECK-NEXT: `-VarDecl {{.*}} var 'N::Integer':'int' cinit
+//CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
Index: test/SemaCXX/referenced_using_declaration_2.cpp
===
--- test/SemaCXX/referenced_using_declaration_2.cpp
+++ test/SemaCXX/referenced_using_declaration_2.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -std=c++11 -ast-dump %s | FileCheck %s --strict-whitespace
+
+namespace N {
+ typedef int Integer;
+ typedef char Char;
+}
+
+using N::Integer;
+using N::Char; // Referenced
+
+void Foo(int p1, N::Integer p2, Char p3) {
+ N::Integer var;
+ var = 0;
+}
+
+using N::Integer; // Referenced
+Integer Bar() {
+ using N::Char;
+ return 0;
+}
+
+//CHECK: |-UsingDecl {{.*}} N::Integer
+//CHECK-NEXT: |-UsingShadowDecl {{.*}} implicit Typedef {{.*}} 'Integer'
+//CHECK-NEXT: | `-TypedefType {{.*}} 'N::Integer' sugar
+//CHECK-NEXT: | |-Typedef {{.*}} 'Integer'
+//CHECK-NEXT: | `-BuiltinType {{.*}} 'int'
+//CHECK-NEXT: |-UsingDecl {{.*}} referenced N::Char
+//CHECK-NEXT: |-UsingShadowDecl {{.*}} implicit referenced Typedef {{.*}} 'Char'
+//CHECK-NEXT: | `-TypedefType {{.*}} 'N::Char' sugar
+//CHECK-NEXT: | |-Typedef {{.*}} 'Char'
+//CHECK-NEXT: | `-BuiltinType {{.*}} 'char'
+//CHECK-NEXT: |-FunctionDecl {{.*}} Foo 'void (int, N::Integer, N::Char)'
+//CHECK-NEXT: | |-ParmVarDecl {{.*}} p1 'int'
+//CHECK-NEXT: | |-ParmVarDecl {{.*}} p2 'N::Integer':'int'
+//CHECK-NEXT: | |-ParmVarDecl {{.*}} p3 'N::Char':'char'
+//CHECK-NEXT: | `-CompoundStmt {{.*}}
+//CHECK-NEXT: | |-DeclStmt {{.*}}
+//CHECK-NEXT: | | `-VarDecl {{.*}} used var 'N::Integer':'int'
+//CHECK-NEXT: | `-BinaryOperator {{.*}} 'N::Integer':'int' lvalue '='
+//CHECK-NEXT: | |-DeclRefExpr {{.*}} 'N::Integer':'int' lvalue Var {{.*}} 'var' 'N::Integer':'int'
+//CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 0
+//CHECK-NEXT: |-UsingDecl {{.*}} referenced N::Integer
+//CHECK-NEXT: |-UsingShadowDecl {{.*}} implicit referenced Typedef {{.*}} 'Integer'
+//CHECK-NEXT: | `-TypedefType {{.*}} 'N::Integer' sugar
+//CHECK-NEXT: | |-Typedef {{.*}} 'Integer'
+//CHECK-NEXT: | `-BuiltinType {{.*}} 'int'
+//CHECK-NEXT:
