|
From: Florian K. <br...@ac...> - 2012-11-18 22:48:05
|
I've changed the code so we no longer need to compile with -Wno-pointer-sign. In doing so I've probably looked at each and every source file(not counting tests) and want to share a few things I noticed. There was widespread confusion about type Char. This is *not* "typedef char Char" as one might expect from the like definitions of Int and Short. Char is "signed char". If you want a plain char, then HChar is the type to use. And you always want a plain char when you're thinking about characters. Also, string literals like "hello world" may be placed into read- only memory. Therefore, one should think about them as arrays of const characters (even though their type is just "array of char" for historical reasons). Therefore, HChar *p = "hi"; is bad. There is at least one place in m_options.c, that I found where we possibly write into a string literal. I put a FIXME there.. There was (and still is) free-wheeling casting between pointer types in many places: - casting away constness - changing signedness of pointer types, i.e. from (Char *) to (UChar *) - unnecessary casts e.g. VG_(memset)((UChar *)&arch->vex_shadow1 or (Char *)NULL - redundant casts, e.g. (void *)p where p has type void * - cast between Addr and void * because we cannot make up our mind (so it seems) Ideally, we should have only very few of those. Besides, casts are detrimental to readability. What I did not do is to replace "int" with "Int" and so forth. While this could be done, it only makes sense to do so, if we can check automatically, that the code stays clean in that respect. GCC won't help with that. Some fancy grepping in an svn hook might. Things I plan to do (on and off) next is: - Enable -Wwrite-strings This will warn about assigning a string literal to pointers where the pointed to area is not const-qualified as in the example above. - Enable -Wcast-qual This will warn about casts that cast away constness. After all, type safety is a good thing. GCC can help. We should take advantage of it. Florian |