address clangd warnings #122
Conversation
| @@ -4,7 +4,7 @@ | |||
| #ifndef AY_CPU_H | |||
| #define AY_CPU_H | |||
|
|
|||
| #include "blargg_endian.h" | |||
There was a problem hiding this comment.
Patch is big. But at least this particular bit is wrong: This header references endian macros, but it now doen't include blargg_endian.h, and in Ay_Cpu.c where it is included blargg_endian.h is included after Ay_Cpu.h
There may be other breakages, too...
There was a problem hiding this comment.
I'm not sure I follow. The header doesn't use blargg_endian.h but the source file does. Wouldn't the build have failed if this weren't the case?
What happens as a result of blargg_endian.h being included after Ay_Cpu.h?
There was a problem hiding this comment.
Build with -Wundef to see: In short, big endian define will be missing and builds on big endian arches will be wrong
There was a problem hiding this comment.
When you say big endian define, are you referring to BLARGG_BIG_ENDIAN?
If so, that's not defined in blargg_endian.h, that gets defined by the build system.
It looks like a lot of these undef warnings could be remedied by simply changing #if to #ifdef/#ifndef
There was a problem hiding this comment.
When you say big endian define, are you referring to
BLARGG_BIG_ENDIAN?
Yes
If so, that's not defined in
blargg_endian.h, that gets defined by the build system.
Well, you're actually correct (and I was the one who did it that way.. face palm..)
It looks like a lot of these undef warnings could be remedied by simply changing
#ifto#ifdef/#ifndef
Be really careful there.
And I hope that you don't include such a changeset in here: One patch, doing only one thing at a time should be the norm.
gme/Blip_Buffer.h
Outdated
| @@ -121,8 +121,6 @@ class Blip_Buffer { | |||
| friend class Blip_Reader; | |||
| }; | |||
|
|
|||
| #include "blargg_config.h" | |||
There was a problem hiding this comment.
Why remove this?
It can be useful when building through standalone makefiles, for e.g.
There was a problem hiding this comment.
Okay, it's been put back in.
| @@ -5,8 +5,7 @@ | |||
| #ifndef GB_CPU_H | |||
| #define GB_CPU_H | |||
|
|
|||
| #include "blargg_common.h" | |||
| #include "blargg_endian.h" | |||
There was a problem hiding this comment.
Same issue noted with Ay_Cpu.h changes above
| @@ -15,8 +13,6 @@ details. You should have received a copy of the GNU Lesser General Public | |||
| License along with this module; if not, write to the Free Software Foundation, | |||
| Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ | |||
|
|
|||
| #include "blargg_source.h" | |||
There was a problem hiding this comment.
Is this really not needed here? (Haven't read carefully this part)
There was a problem hiding this comment.
Maybe it was left over from a time when it was needed.
| @@ -4,8 +4,8 @@ | |||
| #ifndef GB_OSCS_H | |||
| #define GB_OSCS_H | |||
|
|
|||
| #include "blargg_common.h" | |||
| @@ -4,7 +4,7 @@ | |||
| #ifndef KSS_CPU_H | |||
| #define KSS_CPU_H | |||
|
|
|||
| #include "blargg_endian.h" | |||
There was a problem hiding this comment.
same issue noted with Ay_Cpu.h changes noted above
| @@ -5,9 +5,7 @@ | |||
| #define SNES_SPC_H | |||
|
|
|||
| #include "Spc_Dsp.h" | |||
| #include "blargg_endian.h" | |||
There was a problem hiding this comment.
same issue noted with Ay_Cpu.h changes noted above
|
In general, I personally don't like this patch very much, at least as it is. It doesn't fix real issues, but seem to satisfy clang witch hunt only. An includes clean-up like |
or be explicit about using int division.
|
Sorry for not paying attention on this, but, any changes should be done VERY carefully. Sezer pointed the rest of problems already. I'll try to consume this by myself, taking the best of these changes only and ensuring nothing will hurt. |
avoid including headers where they aren't being directly used
in floating point arithmetic, use int multiplication over int division, or be explicit about using division.
use C++-standardized, namespace-aware headers instead of the older C-style headers.