Re: [Mesa-dev] Add an ASSERTED macro to use in place of MAYBE_UNUSED?
On Monday, April 22, 2019 3:21:22 PM PDT Eric Anholt wrote: > Jason Ekstrand writes: > > > All, > > > > I've seen discussions come up several times lately about whether you should > > use MAYBE_UNUSED or UNUSED in what scenario and why do we have two of them > > anyway. That got me thinking a bit. Maybe what we actually want instead > > of MAYBE_UNUSED is something like this: > > > > #ifdef NDEBUG > > #define ASSERTED UNUSED > > #else > > #define ASSERTED > > #endif > > > > That way, if you only need a parameter for asserts, you can declare it > > ASSERTED and it won't warn in release builds will still throw a warning if > > you do a debug build which doesn't use it. Of course, there are other > > times when something is validly MAYBE_UNUSED such as auto-generated code or > > the genX code we use on Intel. However, this provides additional meaning > > and means the compiler warnings are still useful even after you've > > relegated a value to assert-only. > > > > Thoughts? I'm also open to a better name; that's the best I could do in 5 > > minutes. > > We should delete one or the other of the current ones and not have > different names to need to know for the same underlying function > attribute. I'd prefer deleting MAYBE_UNUSED (UNUSED is less typing and > the name of the underlying attribute), but would go either way. I agree, I'd be happy to see MAYBE_UNUSED go. The actual compiler attribute is named "unused". Jason's idea sounds interesting, too, in that it lets us get warnings about unused code in debug builds, but still silence release warnings about values we assert on. I'd be okay with adding one of those and using that. --Ken signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Add an ASSERTED macro to use in place of MAYBE_UNUSED?
On Mon, Apr 22, 2019 at 3:11 PM Matt Turner wrote: > > On Mon, Apr 22, 2019 at 1:09 PM Kristian Høgsberg wrote: > > > > On Mon, Apr 22, 2019 at 12:11 PM Jason Ekstrand > > wrote: > > > > > > All, > > > > > > I've seen discussions come up several times lately about whether you > > > should use MAYBE_UNUSED or UNUSED in what scenario and why do we have two > > > of them anyway. That got me thinking a bit. Maybe what we actually want > > > instead of MAYBE_UNUSED is something like this: > > > > > > #ifdef NDEBUG > > > #define ASSERTED UNUSED > > > #else > > > #define ASSERTED > > > #endif > > > > > > That way, if you only need a parameter for asserts, you can declare it > > > ASSERTED and it won't warn in release builds will still throw a warning > > > if you do a debug build which doesn't use it. Of course, there are other > > > times when something is validly MAYBE_UNUSED such as auto-generated code > > > or the genX code we use on Intel. However, this provides additional > > > meaning and means the compiler warnings are still useful even after > > > you've relegated a value to assert-only. > > > > > > Thoughts? I'm also open to a better name; that's the best I could do in > > > 5 minutes. > > > > I think that's going in the wrong direction - if anything I think that > > having both UNUSED and MAYBE_UNUSED is redundant and feel that just > > UNUSED would be fine. __attribute__((unused)) doesn't mean "strictly > > not used", it means "don't warn if this isn't used". > > I agree that having both UNUSED and MAYBE_UNUSED is silly and I would > be happy to see MAYBE_UNUSED go away. > > I think the advantage of Jason's proposal is that we are alerted if > there is actually dead code. E.g., if we remove the assert that used a > variable, we currently won't get a warning from the compiler that the > variable is unused. At least in release builds we would, if we did > what Jason suggests. > > Maybe we do what Jason suggests and then remove MAYBE_UNUSED? Right... I see now, that's sounds reasonable. Kristian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Add an ASSERTED macro to use in place of MAYBE_UNUSED?
Jason Ekstrand writes: > All, > > I've seen discussions come up several times lately about whether you should > use MAYBE_UNUSED or UNUSED in what scenario and why do we have two of them > anyway. That got me thinking a bit. Maybe what we actually want instead > of MAYBE_UNUSED is something like this: > > #ifdef NDEBUG > #define ASSERTED UNUSED > #else > #define ASSERTED > #endif > > That way, if you only need a parameter for asserts, you can declare it > ASSERTED and it won't warn in release builds will still throw a warning if > you do a debug build which doesn't use it. Of course, there are other > times when something is validly MAYBE_UNUSED such as auto-generated code or > the genX code we use on Intel. However, this provides additional meaning > and means the compiler warnings are still useful even after you've > relegated a value to assert-only. > > Thoughts? I'm also open to a better name; that's the best I could do in 5 > minutes. We should delete one or the other of the current ones and not have different names to need to know for the same underlying function attribute. I'd prefer deleting MAYBE_UNUSED (UNUSED is less typing and the name of the underlying attribute), but would go either way. signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Add an ASSERTED macro to use in place of MAYBE_UNUSED?
On Mon, Apr 22, 2019 at 1:09 PM Kristian Høgsberg wrote: > > On Mon, Apr 22, 2019 at 12:11 PM Jason Ekstrand wrote: > > > > All, > > > > I've seen discussions come up several times lately about whether you should > > use MAYBE_UNUSED or UNUSED in what scenario and why do we have two of them > > anyway. That got me thinking a bit. Maybe what we actually want instead > > of MAYBE_UNUSED is something like this: > > > > #ifdef NDEBUG > > #define ASSERTED UNUSED > > #else > > #define ASSERTED > > #endif > > > > That way, if you only need a parameter for asserts, you can declare it > > ASSERTED and it won't warn in release builds will still throw a warning if > > you do a debug build which doesn't use it. Of course, there are other > > times when something is validly MAYBE_UNUSED such as auto-generated code or > > the genX code we use on Intel. However, this provides additional meaning > > and means the compiler warnings are still useful even after you've > > relegated a value to assert-only. > > > > Thoughts? I'm also open to a better name; that's the best I could do in 5 > > minutes. > > I think that's going in the wrong direction - if anything I think that > having both UNUSED and MAYBE_UNUSED is redundant and feel that just > UNUSED would be fine. __attribute__((unused)) doesn't mean "strictly > not used", it means "don't warn if this isn't used". I agree that having both UNUSED and MAYBE_UNUSED is silly and I would be happy to see MAYBE_UNUSED go away. I think the advantage of Jason's proposal is that we are alerted if there is actually dead code. E.g., if we remove the assert that used a variable, we currently won't get a warning from the compiler that the variable is unused. At least in release builds we would, if we did what Jason suggests. Maybe we do what Jason suggests and then remove MAYBE_UNUSED? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Add an ASSERTED macro to use in place of MAYBE_UNUSED?
On Mon, Apr 22, 2019 at 12:11 PM Jason Ekstrand wrote: > > All, > > I've seen discussions come up several times lately about whether you should > use MAYBE_UNUSED or UNUSED in what scenario and why do we have two of them > anyway. That got me thinking a bit. Maybe what we actually want instead of > MAYBE_UNUSED is something like this: > > #ifdef NDEBUG > #define ASSERTED UNUSED > #else > #define ASSERTED > #endif > > That way, if you only need a parameter for asserts, you can declare it > ASSERTED and it won't warn in release builds will still throw a warning if > you do a debug build which doesn't use it. Of course, there are other times > when something is validly MAYBE_UNUSED such as auto-generated code or the > genX code we use on Intel. However, this provides additional meaning and > means the compiler warnings are still useful even after you've relegated a > value to assert-only. > > Thoughts? I'm also open to a better name; that's the best I could do in 5 > minutes. I think that's going in the wrong direction - if anything I think that having both UNUSED and MAYBE_UNUSED is redundant and feel that just UNUSED would be fine. __attribute__((unused)) doesn't mean "strictly not used", it means "don't warn if this isn't used". > > --Jason > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
