Thank you for the review.
+class kf_mkstemp : public known_function +{ +public: + bool + matches_call_types_p (const call_details &cd) const final override + { + return (cd.num_args () == 1 && cd.arg_is_pointer_p (0)); + } + + void + impl_call_pre (const call_details &cd) const final override + { + region_model_context *ctxt = cd.get_ctxt (); + /* If there's no context we can't store warnings, so we return early. */ + if (!ctxt) + { + cd.set_any_lhs_with_defaults (); + return; + }Hmmm, I don't like the way the patch has cd.set_any_lhs_with_defaults (); above and then repeats it below at the end of the function (DRY principle). How about introducing a subroutine within the class to do the checks, so it looks like: if (ctxt) if (!check_argument (cd)) return cd.set_any_lhs_with_defaults (); or somesuch? Or does that make things messier?
While thinking about this, I realized that calling `ctxt>terminate_path ()` when the argument to `mkstemp` isn't a null-terminated string may be too aggressive. My original code followed the pattern used by `kf_strcat`, but `mkstemp` is more similar to `fopen`. `kf_fopen` sets the LHS to defaults regardless whether `check_for_null_terminated_string_arg` returns NULL. I have updated the code to do the same. Does this seem correct?
If correct, it incidentally means now it's not necessary to return early from `impl_call_pre`, as I have extracted the template string check into a method, as suggested.
diff --git a/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c b/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c new file mode 100644 index 00000000000..d8e0e51c39f --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c @@ -0,0 +1,96 @@ +/* { dg-additional-options "-Wno-analyzer-null-argument" } */ +/* { dg-skip-if "has no putenv" { "avr-*-*" } } */The above skip-if line looks like it was a copy&paste from a test of "putenv"; it means that the test should be skipped on targets matching avr-*-* since they don't have "putenv". There might be targets that don't have mkstemp, but until we discover them, best to delete that line for this test.
Ah yes, sorry, as you can see I based the `mkstemp` tests on the `putenv` tests and wasn't careful enough when updating it.
Other than those nitpicks, the patch looks great; thanks
Thank you. I have another question: I have noticed that generally any non-trivial method is defined out-of-line throughout the codebase. Would it be better to define `kf_mkstemp`'s private methods out-of-line as well?
Tomás
