C++: Rename predicates in FunctionInputsAndOutputs.qll and add QLDoc
#1938
Conversation
cpp/ql/src/semmle/code/cpp/models/interfaces/FunctionInputsAndOutputs.qll
Show resolved
Hide resolved
cpp/ql/src/semmle/code/cpp/models/interfaces/FunctionInputsAndOutputs.qll
Show resolved
Hide resolved
|
|
||
| predicate isInQualifier() { none() } | ||
| predicate isQualifierAddress() { none() } |
There was a problem hiding this comment.
I think the QLDoc should go on these predicates rather than the associated classes. It's these predicates that callers use and get autocomplete for.
I'll so so far as suggesting we deprecate the associated classes (InParameter, etc.). I've never needed them or their toString, so to me they just look like 70 lines of dead boiler-plate code. What's your take on that, @rdmarsh2?
There was a problem hiding this comment.
I've moved the QLDoc to the relevant predicates. However, I'd prefer not to deprecate the classes. If they didn't already exist, I probably wouldn't add them, but they could reasonably be used to filter a set of FunctionInputs or FunctionOutputs in an exists() without a separate condition.
| /** | ||
| * The input value of a parameter to a function. | ||
| * Example: | ||
| * ```cpp |
There was a problem hiding this comment.
I don't believe QLDoc supports language identifiers after triple backticks.
There was a problem hiding this comment.
I'll try a QLDoc build and see what happens.
| class FunctionInput extends TFunctionInput { | ||
| abstract string toString(); | ||
|
|
||
| predicate isInParameter(ParameterIndex index) { none() } | ||
| predicate isParameter(ParameterIndex index) { none() } |
There was a problem hiding this comment.
I'm afraid we can't change these names without keeping the old names as deprecated synonyms. They're used by at least one customer: https://semmle.slack.com/archives/C7NS4H8HY/p1561548291277300
There was a problem hiding this comment.
It also means we need to write a change note about this.
| } | ||
|
|
||
| class OutQualifier extends FunctionOutput, TOutQualifier { | ||
| class OutQualifierObject extends FunctionOutput, TOutQualifierObject { | ||
| override string toString() { result = "OutQualifier" } |
There was a problem hiding this comment.
| override string toString() { result = "OutQualifier" } | |
| override string toString() { result = "OutQualifierObject" } |
There was a problem hiding this comment.
I'm not sure what's going on with the indentation in this edit suggestion. I didn't suggest any change of indentation.
| predicate isReturnValueDeref() { none() } | ||
|
|
||
| /* | ||
| * Holds if this is the output value pointed to by the return value of a function, if the function |
There was a problem hiding this comment.
This comment should have been a QLDoc.
| /** | ||
| * Holds if this is the output value pointed to by the return value of a function, if the function | ||
| * returns a pointer, or the output value referred to by the return value of a function, if the | ||
| * function returns a reference. |
There was a problem hiding this comment.
There are some formatting accidents that are probably cause by the autoformatter acting on a comment that doesn't start with * on every line. Several predicates in this file are affected.
| * `isReturnValueDeref()` holds for the `FunctionOutput` that represents the value of | ||
| * `getReference()` (with type `float`). | ||
| * There is no `FunctionOutput` of `getInt()` for which `isReturnValueDeref()` holds because the | ||
| * return type of `getInt()` is neither a pointer nor a reference. |
There was a problem hiding this comment.
Outside of triple backticks, line breaks are not significant in QLDoc. I think this would read better as a bullet list so all three items don't get run together in a single paragraph. If you prefer them to be run together, I think it's better to reflect that in the source text by running them together there as well.
|
All feedback addressed. Ready for final review and merge. |
No description provided.