This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch, c* ,ObjC*] handle string objects in format checking.
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: IainS <developer at sandoe-acoustics dot co dot uk>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Mike Stump <mrs at gcc dot gnu dot org>
- Date: Sun, 24 Oct 2010 14:26:53 +0000 (UTC)
- Subject: Re: [Patch, c* ,ObjC*] handle string objects in format checking.
- References: <3F894B1C-C5CE-45FD-B73F-843A2C7ED940@sandoe-acoustics.co.uk>
On Sun, 24 Oct 2010, IainS wrote:
> * c-family/c-format.c (format_type): Add gcc_objc_string_format_type.
> (valid_stringptr_type_p): New.
> (handle_format_arg_attribute): Use valid_stringptr_type_p().
> (check_format_string): Likewise.
> (format_types_orig): Add NSString.
> * c-family/c-common.h (objc_string_ref_type_p): New prototype.
(c-family/ has its own ChangeLog.)
Is there something missing here that will be in a followup patch?
You're adding a new format style, "NSString". There's no documentation
for what it means, no testcases for this format style checking, and the
table entry appears to be a dummy table entry. What happens when the
compiler tries to check a call to a function declared to use that type of
format? Are all the NULL table entries safe or will it try to dereference
them? Do these strings appear as STRING_CST? If not, where is the code
that knows how to extract the characters from them for checking?
> +/* Check that we have a pointer to a string, or string object for objc and
> + targets that support them in c*. */
What does "targets that support them in c*" mean? You should explicitly
state what the return value means, and what STRP is.
> + || (*targetcm.string_object_ref_type_p) ((const_tree)strp)));
Missing space in cast.
> + if (! valid_stringptr_type_p (TREE_TYPE (type)))
Excess space in unary !.
> + if (! argument
Likewise.
> Index: gcc/config/darwin.c
> ===================================================================
> --- gcc/config/darwin.c (revision 165889)
> +++ gcc/config/darwin.c (working copy)
> @@ -50,6 +50,7 @@ along with GCC; see the file COPYING3. If not see
> #include "debug.h"
> #include "obstack.h"
> #include "lto-streamer.h"
> +#include "c-family/c-format.h"
There are a few legacy c-* includes in target .c files that aren't
c-family-specific, but you shouldn't add to them in new patches; why can't
this go in darwin-c.c?
--
Joseph S. Myers
joseph@codesourcery.com