GNU bug report logs -
#75451
scratch/igc: Enable CHECK_STRUCTS
Previous Next
Full log
Message #109 received at 75451 <at> debbugs.gnu.org (full text, mbox):
"Paul Eggert" <eggert <at> cs.ucla.edu> writes:
> GCC complained in my builds after some pdumper.c changes were added
> today in this area. I pacified GCC by installing the attached further
Did you build with optimization? -Wmaybe-default doesn't work without
optimization, and apparently clang has no such warning in this case,
either.
@@ -4544,26 +4534,19 @@ dump_anonymous_allocate_w32 (void *base,
# define MAP_ANONYMOUS MAP_ANON
# endif
+static int const mem_prot_posix_table[] = {
+ [DUMP_MEMORY_ACCESS_NONE] = PROT_NONE,
+ [DUMP_MEMORY_ACCESS_READ] = PROT_READ,
+ [DUMP_MEMORY_ACCESS_READWRITE] = PROT_READ | PROT_WRITE,
+};
+
static void *
dump_anonymous_allocate_posix (void *base,
size_t size,
enum dump_memory_protection protection)
{
void *ret;
- int mem_prot;
-
- switch (protection)
- {
- case DUMP_MEMORY_ACCESS_NONE:
- mem_prot = PROT_NONE;
- break;
- case DUMP_MEMORY_ACCESS_READ:
- mem_prot = PROT_READ;
- break;
- case DUMP_MEMORY_ACCESS_READWRITE:
- mem_prot = PROT_READ | PROT_WRITE;
- break;
- }
+ int mem_prot = mem_prot_posix_table[protection];
int mem_flags = MAP_PRIVATE | MAP_ANONYMOUS;
if (mem_prot != PROT_NONE)
If you think the static table is more readable and not too limiting,
this is perfectly fine.
But:
1. we lose the -Wswitch warning entirely this way. The old code would
have generated it had we simply specified -Wswitch-enum.
2. GCC generates an equivalent table itself when building with
optimization, if it can do so.
> patch. Although this is not quite as good as far as static checking
> goes, the loss in checking is small and anyway --enable-gcc-warnings
> builds succeed now.
The specific mem_prot case doesn't benefit from too much enum checking
(it's an internal enum, not even one in a header file), so I agree the
loss is quite small.
But wasn't the point of the initial change by Stefan Kangas to add new
warnings if the enum is changed? We've lost those warnings again, by
using arrays, and I don't see a way to restore them easily.
I fear turning a switch statement into an array of function pointers
makes the code less readable, hurts debugging, and makes it more likely
GCC won't be able to turn the function pointer call into a direct call
when optimizing for machines where this is beneficial.
The warnings are annoying, so maybe we should go back to the "default:
emacs_abort();" solution for now, or switch to "default: eassume (0);"
*if* we're really sure that we won't see strange crashes due to
out-of-bounds array accesses.
My impression is Stefan Kangas is fixing this particular case in order
to find a more general pattern to follow when fixing the rest of the
code. I don't think we've settled on one yet; note that simply removing
"default: emacs_abort();" may enable compile-time warnings, but we lose
runtime errors which would have informed of us of the same problem,
making behavior in such cases erratic.
For a permanent solution, I still think we should enable -Wswitch-enum.
Unnecessary and unavoidable warnings can be avoided by casting to int or
typeof (CASE_LABEL_VALUE) in the switch statement, and bidi.c can just
add a #pragma to disable this warning entirely for its many switches
over enumerations of unicode character classes.
Unfortunately, I started doing that and noticed several cases in which I
think the -Wswitch-enum warning may be appropriate. I'd like to
investigate those further, but it'd be counterproductive to silence a
warning introduced to warn us about errors if it is actually triggered
by an error which should be fixed :-)
Pip
This bug report was last modified 105 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.