drobilla / jalv

A simple fully-featured host for LV2 plugins
http://drobilla.net/software/jalv
ISC License
52 stars 18 forks source link

Fix compiler warnings with additional gcc flags #14

Closed twischer-adit closed 5 years ago

twischer-adit commented 6 years ago

JALV was build with the following command $ ./waf build --cflags "-Wall -Werror -Wextra -Wformat -Wno-format- nonliteral -Wformat-security -Wformat-y2k"

to find possible critical issues. To avoid to recheck the same warnings always by hand I would like to fix also this non critical warnings.

drobilla commented 6 years ago

Use -Wno-unused-parameter, I don't litter my code with all those messy void casts to avoid a warning that can simply be turned off.

drobilla commented 6 years ago

I don't understand the purpose of this configure parameter since waf will obey CFLAGS by default, which is the standard way to do things. The ones I personally use are set with --strict for convenience, but you can use whatever flags you like (provided --debug isn't given, which clears them).

twischer-adit commented 6 years ago

Use -Wno-unused-parameter, I don't litter my code with all those messy void casts to avoid a warning that can simply be turned off.

The build system of my company is enforcing -Wunused-parameter because some of these warnings are real issues and have already avoided time consuming debug sessions (e.g. a similar member variable exists). Would you be happy with a definition like the following?

#ifndef ATTRIBUTE_UNUSED
#define ATTRIBUTE_UNUSED __attribute__ ((__unused__))
#endif

and a usage like this

static int snd_pcm_hw_mmap(snd_pcm_t *pcm ATTRIBUTE_UNUSED)
{

See http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=include/global.h;h=d73d333aae19d916243a7a87343cb55705652a2b;hb=HEAD#l49 and http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_hw.c;h=59a242009e9f5f1098759305499eaa27bef6c1ab;hb=HEAD#l1069

I don't understand the purpose of this configure parameter since waf will obey CFLAGS by default, which is the standard way to do things

Thanks for the hint. I have double checked it again. My issue was that I only provided my {{CFLAGS}} for building but not for configuring. Therefore my additional {{--cflags}} option is superfluous and I will remove it.

But could you first please give your opinion about {{ATTRIBUTE_UNUSED}}.

drobilla commented 6 years ago

I'm coming around to making code -Wunused-parameter clean, even though it's so noisy and annoying in C (better in C++ where you can just leave the names out).

I guess using an unused attribute is a better solution, since the compiler can check that it's actually unused unlike the void cast. Having such a macro redundantly defined in a ton of places across the "LV2 ecosystem" is not a very pleasant thought, but I guess we'll just have to live with that (... or abuse the LV2 headers to include one there somewhere.. hmm..)

So... somewhat reluctant acceptance, I guess? :)

twischer-adit commented 6 years ago

Having such a macro redundantly defined in a ton of places across the "LV2 ecosystem" is not a very pleasant thought, but I guess we'll just have to live with that

I think the zix/commom.h header would be a good place to add such a define. This header will already be included by nearly all source files. So should I change anything or would you like to do it by your own?

drobilla commented 6 years ago

Hmm... if there's such warnings in zix, which there probably are (if maybe not in the bits included in Jalv), that's a good idea.

I can do it, but by all means, feel free if you like :)

twischer-adit commented 5 years ago

Okay. I would add

#ifndef ATTRIBUTE_UNUSED
#define ATTRIBUTE_UNUSED __attribute__ ((__unused__))
#endif

to the zix/common.h file and use this it where required. You are fine with this approach?

drobilla commented 5 years ago

Sounds good, except please add a ZIX prefix (bit annoying in this case but I think it's good to stick to strict namespacing)

twischer-adit commented 5 years ago

May be shorten it to ZIX_UNUSED?

drobilla commented 5 years ago

:+1:

twischer-adit commented 5 years ago

I have updated this PR

drobilla commented 5 years ago

Is _GNUC actually defined for you? That doesn't work for me.

I pushed some fixups that fix this, and puts the attribute first which I find more pleasant to read and seems to work fine.

twischer-adit commented 5 years ago

Is _GNUC actually defined for you? That doesn't work for me.

Yes it was working for my built environment. But GNUC is also working for me. There is only one compiler warning which can be solved with my additional fixup.

But I have one off-topic questions: Where will jalv_ui_resize() be called? (I could not find any place) May be we can remove this function completely?

drobilla commented 5 years ago

Ah, right, whoops.