From 8a435ae3214843a4775fee42bfd0d59f5590469f Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 11 Jan 2019 11:02:52 +0100 Subject: [PATCH 1/4] C++: Autoformat "Declaration hides parameter" --- .../Hiding/DeclarationHidesParameter.ql | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql b/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql index 54b59d3f5c2f..afd1d07ece3a 100644 --- a/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql +++ b/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql @@ -8,29 +8,29 @@ * @tags maintainability * readability */ -import cpp +import cpp -/* Names of parameters in the implementation of a function. - Notice that we need to exclude parameter names used in prototype - declarations and only include the ones from the actual definition. - We also exclude names from functions that have multiple definitions. - This should not happen in a single application but since we - have a system wide view it is likely to happen for instance for - the main function. */ +// Names of parameters in the implementation of a function. +// Notice that we need to exclude parameter names used in prototype +// declarations and only include the ones from the actual definition. +// We also exclude names from functions that have multiple definitions. +// This should not happen in a single application but since we +// have a system wide view it is likely to happen for instance for +// the main function. ParameterDeclarationEntry functionParameterNames(Function f, string name) { exists(FunctionDeclarationEntry fe | - result.getFunctionDeclarationEntry() = fe - and fe.getFunction() = f - and fe.getLocation() = f.getDefinitionLocation() - and strictcount(f.getDefinitionLocation()) = 1 - and result.getName() = name + result.getFunctionDeclarationEntry() = fe and + fe.getFunction() = f and + fe.getLocation() = f.getDefinitionLocation() and + strictcount(f.getDefinitionLocation()) = 1 and + result.getName() = name ) } from Function f, LocalVariable lv, ParameterDeclarationEntry pde -where f = lv.getFunction() and - pde = functionParameterNames(f, lv.getName()) and - not lv.isInMacroExpansion() -select lv, "Local variable '"+ lv.getName() +"' hides a $@.", - pde, "parameter of the same name" +where + f = lv.getFunction() and + pde = functionParameterNames(f, lv.getName()) and + not lv.isInMacroExpansion() +select lv, "Local variable '" + lv.getName() + "' hides a $@.", pde, "parameter of the same name" From 2268f1fee6bc38618ff539b0396467baaec7826a Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 11 Jan 2019 11:06:18 +0100 Subject: [PATCH 2/4] C++: Speed up "Declaration hides parameter" Bad magic ended up in `LocalVariable.getFunction` and effectively created a Cartesian product. Before this change, the timing looked like this: Variable::LocalVariable::getFunction_dispred#bb ... 50.1s #select#cpe#123#fff ............................... 20.6s After this change, those predicates become much faster: Variable::LocalVariable::getFunction_dispred#ff ... 121ms DeclarationHidesParameter::localVariableNames#fff . 77ms #select#cpe#123#fff ............................... 28ms Introducing the predicate `localVariableNames` ensures that we can do the main join on two columns simultaneously, so that's a change we should keep even if we remove the `pragma[nomagic]` later. --- .../Hiding/DeclarationHidesParameter.ql | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql b/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql index afd1d07ece3a..90a9b24daf51 100644 --- a/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql +++ b/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql @@ -28,9 +28,15 @@ ParameterDeclarationEntry functionParameterNames(Function f, string name) { ) } -from Function f, LocalVariable lv, ParameterDeclarationEntry pde +pragma[nomagic] +LocalVariable localVariableNames(Function f, string name) { + name = result.getName() and + f = result.getFunction() +} + +from Function f, LocalVariable lv, ParameterDeclarationEntry pde, string name where - f = lv.getFunction() and - pde = functionParameterNames(f, lv.getName()) and + lv = localVariableNames(f, name) and + pde = functionParameterNames(f, name) and not lv.isInMacroExpansion() select lv, "Local variable '" + lv.getName() + "' hides a $@.", pde, "parameter of the same name" From b38ca944f4d9afaa6740b10fceb4111a5263698e Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 11 Jan 2019 11:26:43 +0100 Subject: [PATCH 3/4] C++: Work around CPP-331 This change suppresses results from "Declaration hides parameter" where the ParameterDeclarationEntry does not link up to the right FunctionDeclarationEntry. --- cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql b/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql index 90a9b24daf51..aaa56e3ff41e 100644 --- a/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql +++ b/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql @@ -23,6 +23,7 @@ ParameterDeclarationEntry functionParameterNames(Function f, string name) { result.getFunctionDeclarationEntry() = fe and fe.getFunction() = f and fe.getLocation() = f.getDefinitionLocation() and + result.getFile() = fe.getFile() and // Work around CPP-331 strictcount(f.getDefinitionLocation()) = 1 and result.getName() = name ) From b65e2f8b79d8806656446ab0550e8900b1036fef Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 11 Jan 2019 14:07:22 +0100 Subject: [PATCH 4/4] C++: Put QLDoc on two helper predicates --- .../Hiding/DeclarationHidesParameter.ql | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql b/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql index aaa56e3ff41e..91317c53af85 100644 --- a/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql +++ b/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql @@ -11,13 +11,14 @@ import cpp -// Names of parameters in the implementation of a function. -// Notice that we need to exclude parameter names used in prototype -// declarations and only include the ones from the actual definition. -// We also exclude names from functions that have multiple definitions. -// This should not happen in a single application but since we -// have a system wide view it is likely to happen for instance for -// the main function. +/** + * Gets the parameter of `f` with name `name`, which has to come from the + * _definition_ of `f` and not a prototype declaration. + * We also exclude names from functions that have multiple definitions. + * This should not happen in a single application but since we + * have a system wide view it is likely to happen for instance for + * the main function. + */ ParameterDeclarationEntry functionParameterNames(Function f, string name) { exists(FunctionDeclarationEntry fe | result.getFunctionDeclarationEntry() = fe and @@ -29,6 +30,7 @@ ParameterDeclarationEntry functionParameterNames(Function f, string name) { ) } +/** Gets a local variable in `f` with name `name`. */ pragma[nomagic] LocalVariable localVariableNames(Function f, string name) { name = result.getName() and