Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++: Improve parameter naming #17778

Merged
merged 1 commit into from
Oct 17, 2024
Merged

C++: Improve parameter naming #17778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/Parameter.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ class Parameter extends LocalScopeVariable, @parameter {
}

private VariableDeclarationEntry getANamedDeclarationEntry() {
result = this.getAnEffectiveDeclarationEntry() and result.getName() != ""
result = this.getAnEffectiveDeclarationEntry() and
exists(string name | var_decls(unresolveElement(result), _, _, name, _) | name != "")
Comment on lines -76 to +77
Copy link
Contributor Author

@jketema jketema Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is needed to avoid non-monotonic recursion after extending VariableDeclarationEntry.getName(). It does not affect the ouput.

}

/**
Expand Down
20 changes: 8 additions & 12 deletions cpp/ql/lib/semmle/code/cpp/Variable.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ class VariableDeclarationEntry extends DeclarationEntry, @var_decl {
name != "" and result = name
or
name = "" and result = this.getVariable().(LocalVariable).getName()
or
name = "" and
not this instanceof ParameterDeclarationEntry and
result = this.getVariable().(Parameter).getName()
Comment on lines +244 to +247
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the missing string representation I found while working on requires expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the internal PR associated with #17740

)
)
}
Expand Down Expand Up @@ -295,19 +299,11 @@ class ParameterDeclarationEntry extends VariableDeclarationEntry {

private string getAnonymousParameterDescription() {
not exists(this.getName()) and
exists(string idx |
idx =
((this.getIndex() + 1).toString() + "th")
.replaceAll("1th", "1st")
.replaceAll("2th", "2nd")
.replaceAll("3th", "3rd")
.replaceAll("11st", "11th")
.replaceAll("12nd", "12th")
.replaceAll("13rd", "13th") and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will confess to being a bit sad that we lose this wonderful, wonky logic.

exists(string anon |
anon = "(unnamed parameter " + this.getIndex().toString() + ")" and
if exists(this.getCanonicalName())
then
result = "declaration of " + this.getCanonicalName() + " as anonymous " + idx + " parameter"
else result = "declaration of " + idx + " parameter"
then result = "declaration of " + this.getCanonicalName() + " as " + anon
else result = "declaration of " + anon
Comment on lines -298 to +306
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This make the string representation of parameter declaration entries consistent with the representation of parameters, which it wasn't.

)
}

Expand Down
4 changes: 2 additions & 2 deletions cpp/ql/test/library-tests/declarationEntry/declarationEntry/roundTrip.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
| declarationEntry.cpp:39:7:39:7 | declaration of operator= | declarationEntry.cpp:39:7:39:7 | operator= | yes |
| declarationEntry.cpp:39:7:39:13 | definition of myClass | declarationEntry.cpp:39:7:39:13 | myClass | yes |
| declarationEntry.cpp:42:6:42:21 | definition of myMemberVariable | declarationEntry.cpp:42:6:42:21 | myMemberVariable | yes |
| file://:0:0:0:0 | declaration of 1st parameter | file://:0:0:0:0 | (unnamed parameter 0) | yes |
| file://:0:0:0:0 | declaration of 1st parameter | file://:0:0:0:0 | (unnamed parameter 0) | yes |
| file://:0:0:0:0 | declaration of (unnamed parameter 0) | file://:0:0:0:0 | (unnamed parameter 0) | yes |
| file://:0:0:0:0 | declaration of (unnamed parameter 0) | file://:0:0:0:0 | (unnamed parameter 0) | yes |
| file://:0:0:0:0 | definition of fp_offset | file://:0:0:0:0 | fp_offset | yes |
| file://:0:0:0:0 | definition of gp_offset | file://:0:0:0:0 | gp_offset | yes |
| file://:0:0:0:0 | definition of overflow_arg_area | file://:0:0:0:0 | overflow_arg_area | yes |
Expand Down
8 changes: 4 additions & 4 deletions cpp/ql/test/library-tests/declarationEntry/more/declarationEntry.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
| file://:0:0:0:0 | declaration of 1st parameter |
| file://:0:0:0:0 | declaration of 1st parameter |
| file://:0:0:0:0 | declaration of 1st parameter |
| file://:0:0:0:0 | declaration of 1st parameter |
| file://:0:0:0:0 | declaration of (unnamed parameter 0) |
| file://:0:0:0:0 | declaration of (unnamed parameter 0) |
| file://:0:0:0:0 | declaration of (unnamed parameter 0) |
| file://:0:0:0:0 | declaration of (unnamed parameter 0) |
| file://:0:0:0:0 | definition of fp_offset |
| file://:0:0:0:0 | definition of gp_offset |
| file://:0:0:0:0 | definition of overflow_arg_area |
Expand Down
16 changes: 8 additions & 8 deletions cpp/ql/test/library-tests/parameters/toStrings/params.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
| test.c:2:8:2:10 | declaration of 1st parameter |
| test.c:2:13:2:15 | declaration of 2nd parameter |
| test.c:2:18:2:20 | declaration of 3rd parameter |
| test.c:2:23:2:25 | declaration of 4th parameter |
| test.c:3:8:3:10 | declaration of y1 as anonymous 1st parameter |
| test.c:3:13:3:15 | declaration of y2 as anonymous 2nd parameter |
| test.c:3:18:3:20 | declaration of y3 as anonymous 3rd parameter |
| test.c:3:23:3:25 | declaration of y4 as anonymous 4th parameter |
| test.c:2:8:2:10 | declaration of (unnamed parameter 0) |
| test.c:2:13:2:15 | declaration of (unnamed parameter 1) |
| test.c:2:18:2:20 | declaration of (unnamed parameter 2) |
| test.c:2:23:2:25 | declaration of (unnamed parameter 3) |
| test.c:3:8:3:10 | declaration of y1 as (unnamed parameter 0) |
| test.c:3:13:3:15 | declaration of y2 as (unnamed parameter 1) |
| test.c:3:18:3:20 | declaration of y3 as (unnamed parameter 2) |
| test.c:3:23:3:25 | declaration of y4 as (unnamed parameter 3) |
| test.c:4:12:4:13 | declaration of x1 |
| test.c:4:20:4:21 | declaration of x2 |
| test.c:4:28:4:29 | declaration of x3 |
Expand Down
4 changes: 2 additions & 2 deletions cpp/ql/test/query-tests/Metrics/Dependencies/dependencies.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
| file://:0:0:0:0 | declaration of 1st parameter | LibB/libb_internal.h:5:8:5:12 | thing |
| file://:0:0:0:0 | declaration of 1st parameter | LibB/libb_internal.h:5:8:5:12 | thing |
| file://:0:0:0:0 | declaration of (unnamed parameter 0) | LibB/libb_internal.h:5:8:5:12 | thing |
| file://:0:0:0:0 | declaration of (unnamed parameter 0) | LibB/libb_internal.h:5:8:5:12 | thing |
| include.h:3:25:3:33 | num | LibD/libd.h:5:12:5:14 | num |
| main.cpp:8:31:8:31 | call to container | LibC/libc.h:9:3:9:3 | container |
| main.cpp:8:31:8:31 | definition of x | LibB/libb_internal.h:5:8:5:12 | thing |
Expand Down
Loading