4 ms·

> Most C++ compilers have them, the problem is the macho coders that think that know best. If you mean the warnings, it's because they're way over the top.

by byuu 10y ago

> Most C++ compilers have them, the problem is the macho coders that think that know best. If you mean the warnings, it's because they're way over the top. switch(enabled) > warning: switch on bool switch(static_cast<int>(enabled)) Sigh. Now I know, "why would you ever switch on a bool?! It's only two possible states!" Because it was in an ARM CPU emulator that decoded three to five fields per instruction, most of which were multiple bits long, but a few were just one bit. It gave a structured, consistent look to the code for each field to be implemented with a switch. That's just one example, though. My current list of, "are you kidding me?" warning disables for Clang are: #pragma clang diagnostic ignored "-Wunknown-pragmas" #pragma clang diagnostic ignored "-Wempty-body" #pragma clang diagnostic ignored "-Wparentheses" #pragma clang diagnostic ignored "-Wreturn-type" #pragma clang diagnostic ignored "-Wswitch" #pragma clang diagnostic ignored "-Wswitch-bool" #pragma clang diagnostic ignored "-Wtautological-compare" #pragma clang diagnostic ignored "-Wabsolute-value" This was all for perfectly legal, unambiguous code. Anything even remotely suspicious I'd fix. You probably still think I'm a macho coder anyway, but I'm honestly not. I'm just rather obsessive about consistency and simplicity in my code. I would really not enjoy working on a codebase that forced me to use -Wall -Wextra -Werr.

twoodfin 10y ago
Why disable tautological-compare? Of all the pickier warnings over the years, anecdotally I'd say it's been one that's caught an outsized number of actual bugs. >= 0 on an unsigned value is the classic one.
- byuu 10y ago
  It triggers a lot in generic code (eg templates), where it's not so obvious the result is going to be tautalogical until you instantiate the template. Another time I've used it is just pure aesthetics: if(addr >= 0x0000 && addr <= 0x7fff) return ram.read(addr); if(addr >= 0x8000 && addr <= 0xbfff) return cpu.read(addr); if(addr >= 0xc000 && addr <= 0xffff) return apu.read(addr); Looks nicer than: if(addr <= 0x7fff) return ram.read(addr); if(addr >= 0x8000 && addr <= 0xbfff) return cpu.read(addr); if(addr >= 0xc000 && addr <= 0xffff) return apu.read(addr); And the compiler will delete the unnecessary comparison anyway, so there's no great reason to warn about it. And now imagine you want to make it even nicer, so you add between(addr, lo, hi) as a macro: (note: not a real example -- I wouldn't do this for this case, just for demonstrative purposes.) if(between(addr, 0x0000, 0x7fff)) ...; if(between(addr, 0x8000, 0xbfff)) ...; if(between(addr, 0xc000, 0xffff)) ...; Now you have the generic code triggering the warnings again.
poizan42 10y ago
-Wparentheses is the one that prevents if (password_ok = true) { ...
- byuu 10y ago
  I'm aware, but that's perfectly legal code. Every warning case I've described can result in bugs, but the problem is when you intended to do what you did, you can't get the compiler to shut up about it there. For the warnings I disable, they're things I do all the time. I can't recall ever typing = when I meant ==; whereas I combine assign + test into a single statement very frequently. The warnings I keep on are the ones where I'd never want to do those things, and it'd never be proper code. Those are great warnings to have in place.
  - Too 10y ago
    if (password_ok = true) is not perfectly legal code in any serious project. It's not worth the risk to allow it just to save one line of code.
delroth 10y ago
I take the alternate approach to this, which is to let the compiler warnings (and other related tooling, like static analyzers) drive how I write the details of my code. I think it leads to a higher average code quality than the micromanagement approach where the details need to be perfect. In particular, as soon as you have multiple contributors, people tend to have inconsistencies in how they prefer doing things. Having some tooling enforcing all this really helps in killing unproductive arguments :)
deleted 10y ago
[deleted]
to3m 10y ago
Why do you switch off -Wreturn-type?
- byuu 10y ago
  Because it warns even on unreachable cases. weird_non_trivial_copy_constructor_type foo() { switch(value & 3) { case 0: return a; case 1: return b; case 2: return c; case 3: return d; } } You can fix it by adding a default:, which will generate worse code; or by adding a dummy return (usually easy, but not always) after the switch; or by using compiler-specific intrinsics like __atribute__((unreachable)). Or you can turn off the warning. If they add [[unreachable]] to the standard, then I'll consider switching to that and re-enabling this warning. Because I'll admit, I have made the mistake of omitting a return type before.
  - to3m 10y ago
    Right, yes. I guess a 4-value enum could be an option too, but I've never tried. For gcc and clang I've also had some success with the diagnostic pragmas, which I stick in pshpackN.h-/poppack.h-style header files. For example, I use these to stop both moaning about flexible array members in C++ code. Push header: #ifdef __GNUC__ #pragma GCC diagnostic push #ifdef __clang__ #pragma GCC diagnostic ignored "-Wc99-extensions" #else #pragma GCC diagnostic ignored "-Wpedantic" #endif #endif Pop header: #ifdef __GNUC__ #pragma GCC diagnostic pop #endif

by byuu 10y ago

twoodfin 10y ago

Why disable tautological-compare? Of all the pickier warnings over the years, anecdotally I'd say it's been one that's caught an outsized number of actual bugs. >= 0 on an unsigned value is the classic one.

byuu 10y ago
It triggers a lot in generic code (eg templates), where it's not so obvious the result is going to be tautalogical until you instantiate the template. Another time I've used it is just pure aesthetics: if(addr >= 0x0000 && addr <= 0x7fff) return ram.read(addr); if(addr >= 0x8000 && addr <= 0xbfff) return cpu.read(addr); if(addr >= 0xc000 && addr <= 0xffff) return apu.read(addr); Looks nicer than: if(addr <= 0x7fff) return ram.read(addr); if(addr >= 0x8000 && addr <= 0xbfff) return cpu.read(addr); if(addr >= 0xc000 && addr <= 0xffff) return apu.read(addr); And the compiler will delete the unnecessary comparison anyway, so there's no great reason to warn about it. And now imagine you want to make it even nicer, so you add between(addr, lo, hi) as a macro: (note: not a real example -- I wouldn't do this for this case, just for demonstrative purposes.) if(between(addr, 0x0000, 0x7fff)) ...; if(between(addr, 0x8000, 0xbfff)) ...; if(between(addr, 0xc000, 0xffff)) ...; Now you have the generic code triggering the warnings again.

poizan42 10y ago

-Wparentheses is the one that prevents if (password_ok = true) { ...

byuu 10y ago
I'm aware, but that's perfectly legal code. Every warning case I've described can result in bugs, but the problem is when you intended to do what you did, you can't get the compiler to shut up about it there. For the warnings I disable, they're things I do all the time. I can't recall ever typing = when I meant ==; whereas I combine assign + test into a single statement very frequently. The warnings I keep on are the ones where I'd never want to do those things, and it'd never be proper code. Those are great warnings to have in place.
- Too 10y ago
  if (password_ok = true) is not perfectly legal code in any serious project. It's not worth the risk to allow it just to save one line of code.

delroth 10y ago

I take the alternate approach to this, which is to let the compiler warnings (and other related tooling, like static analyzers) drive how I write the details of my code. I think it leads to a higher average code quality than the micromanagement approach where the details need to be perfect. In particular, as soon as you have multiple contributors, people tend to have inconsistencies in how they prefer doing things. Having some tooling enforcing all this really helps in killing unproductive arguments :)

deleted 10y ago

[deleted]

to3m 10y ago

Why do you switch off -Wreturn-type?

byuu 10y ago
Because it warns even on unreachable cases. weird_non_trivial_copy_constructor_type foo() { switch(value & 3) { case 0: return a; case 1: return b; case 2: return c; case 3: return d; } } You can fix it by adding a default:, which will generate worse code; or by adding a dummy return (usually easy, but not always) after the switch; or by using compiler-specific intrinsics like __atribute__((unreachable)). Or you can turn off the warning. If they add [[unreachable]] to the standard, then I'll consider switching to that and re-enabling this warning. Because I'll admit, I have made the mistake of omitting a return type before.
- to3m 10y ago
  Right, yes. I guess a 4-value enum could be an option too, but I've never tried. For gcc and clang I've also had some success with the diagnostic pragmas, which I stick in pshpackN.h-/poppack.h-style header files. For example, I use these to stop both moaning about flexible array members in C++ code. Push header: #ifdef __GNUC__ #pragma GCC diagnostic push #ifdef __clang__ #pragma GCC diagnostic ignored "-Wc99-extensions" #else #pragma GCC diagnostic ignored "-Wpedantic" #endif #endif Pop header: #ifdef __GNUC__ #pragma GCC diagnostic pop #endif