Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some compiler warnings #917

Merged
merged 6 commits into from
Jul 7, 2017
Merged

Fix some compiler warnings #917

merged 6 commits into from
Jul 7, 2017

Conversation

smcv
Copy link
Contributor

@smcv smcv commented Mar 12, 2017

Please see individual commits for details.

If any of the individual patches are not desired, the others can be cherry-picked.

@ensiform
Copy link
Member

I'd like an extra NORETURN defined specifically for func ptrs to avoid the attribute in regular code.

NORETURN_PTR maybe?

@smcv
Copy link
Contributor Author

smcv commented Mar 12, 2017

I'd like an extra NORETURN defined specifically for func ptrs to avoid the attribute in regular code.

Sure, I'll add NORETURN_PTR.

@smcv
Copy link
Contributor Author

smcv commented Mar 12, 2017

Branch rebased and updated.

@smcv
Copy link
Contributor Author

smcv commented Jul 6, 2017

I've done as requested in @ensiform's review. Please could someone take another look at this?

@Razish
Copy link
Member

Razish commented Jul 6, 2017

This looks good to me. Next reviewer can go ahead and merge.

Copy link
Member

@xycaleth xycaleth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the NORETURN and NORETUTN_PTR definitions to shared/qcommon/q_platform.h? It would make more sense I think to move it there since it's platform/implementation dependent.

smcv added 6 commits July 6, 2017 16:20
It issues compiler warnings (or did until recently), and git grep says
nothing actually calls or mentions this function.

Signed-off-by: Simon McVittie <smcv@debian.org>
clang can see that BufferRead() does not always assign to length,
issuing this warning:

IcarusImplementation.cpp|601 col 13| warning: ‘length’ may be used uninitialized in this function [-Wmaybe-uninitialized]

(This only happens if the buffer is null, in which case we have
bigger problems.)

Signed-off-by: Simon McVittie <smcv@debian.org>
According to comments in code/game/g_public.h (whose version was
already tagged noreturn), for function pointers this only works with
the gcc/clang-specific spelling __attribute__((noreturn)), and not with
the gcc/clang/MSVC abstraction NORETURN. For function implementations,
we can use NORETURN to get MSVC support too.

This lets the compiler reason about what returns and what doesn't,
and in particular silences gcc warnings about functions that
are declared NORETURN but that did appear to return. They didn't,
because they ended with a call to (a function pointer that
eventually calls) Com_Error, but without these annotations
the compiler couldn't know that.

Signed-off-by: Simon McVittie <smcv@debian.org>
Signed-off-by: Simon McVittie <smcv@debian.org>
This expands to the same thing as NORETURN on gcc (and clang), but
expands to nothing on MSVC.

Signed-off-by: Simon McVittie <smcv@debian.org>
Signed-off-by: Simon McVittie <smcv@debian.org>
@smcv
Copy link
Contributor Author

smcv commented Jul 7, 2017

@xycaleth: better now?

I initially put NORETURN_PTR in the same place as NORETURN, but moving them both to shared does seem nicer.

For a lot of qcommon.h and q_shared.h there's a balancing act between sharing as much as possible between MP and SP, and keeping those files similar to the ones in ioquake3 for easier merging; but NORETURN is a new thing in OpenJK so that reasoning doesn't matter.

@xycaleth
Copy link
Member

xycaleth commented Jul 7, 2017

Much better :)

@xycaleth xycaleth merged commit 18324ba into JACoders:master Jul 7, 2017
Blackwolf1337 pushed a commit to Blackwolf1337/JaVisionPlus that referenced this pull request Jul 15, 2017
deathsythe47 pushed a commit to jkanewmod/NewJK that referenced this pull request Aug 18, 2018
* Consistently declare Com_Error implementations as noreturn

According to comments in code/game/g_public.h (whose version was
already tagged noreturn), for function pointers this only works with
the gcc/clang-specific spelling __attribute__((noreturn)), and not with
the gcc/clang/MSVC abstraction NORETURN. For function implementations,
we can use NORETURN to get MSVC support too.

This lets the compiler reason about what returns and what doesn't,
and in particular silences gcc warnings about functions that
are declared NORETURN but that did appear to return. They didn't,
because they ended with a call to (a function pointer that
eventually calls) Com_Error, but without these annotations
the compiler couldn't know that.

Signed-off-by: Simon McVittie <smcv@debian.org>

* AI_ClosestGroupEntityNumToPoint: Remove, unused

It issues compiler warnings (or did until recently), and git grep says
nothing actually calls or mentions this function.

Signed-off-by: Simon McVittie <smcv@debian.org>

* Icarus: Squash a compiler warning about possibly uninitialized length

clang can see that BufferRead() does not always assign to length,
issuing this warning:

IcarusImplementation.cpp|601 col 13| warning: ‘length’ may be used uninitialized in this function [-Wmaybe-uninitialized]

(This only happens if the buffer is null, in which case we have
bigger problems.)

Signed-off-by: Simon McVittie <smcv@debian.org>

* Move definition of NORETURN to q_platform.h

Signed-off-by: Simon McVittie <smcv@debian.org>

* Introduce NORETURN_PTR, a version of NORETURN for function pointers

This expands to the same thing as NORETURN on gcc (and clang), but
expands to nothing on MSVC.

Signed-off-by: Simon McVittie <smcv@debian.org>

* Define NORETURN, NORETURN_PTR for non-gcc non-MSVC compilers

Signed-off-by: Simon McVittie <smcv@debian.org>
deathsythe47 pushed a commit to jkanewmod/NewJK that referenced this pull request Aug 18, 2018
* Consistently declare Com_Error implementations as noreturn

According to comments in code/game/g_public.h (whose version was
already tagged noreturn), for function pointers this only works with
the gcc/clang-specific spelling __attribute__((noreturn)), and not with
the gcc/clang/MSVC abstraction NORETURN. For function implementations,
we can use NORETURN to get MSVC support too.

This lets the compiler reason about what returns and what doesn't,
and in particular silences gcc warnings about functions that
are declared NORETURN but that did appear to return. They didn't,
because they ended with a call to (a function pointer that
eventually calls) Com_Error, but without these annotations
the compiler couldn't know that.

Signed-off-by: Simon McVittie <smcv@debian.org>

* AI_ClosestGroupEntityNumToPoint: Remove, unused

It issues compiler warnings (or did until recently), and git grep says
nothing actually calls or mentions this function.

Signed-off-by: Simon McVittie <smcv@debian.org>

* Icarus: Squash a compiler warning about possibly uninitialized length

clang can see that BufferRead() does not always assign to length,
issuing this warning:

IcarusImplementation.cpp|601 col 13| warning: ‘length’ may be used uninitialized in this function [-Wmaybe-uninitialized]

(This only happens if the buffer is null, in which case we have
bigger problems.)

Signed-off-by: Simon McVittie <smcv@debian.org>

* Move definition of NORETURN to q_platform.h

Signed-off-by: Simon McVittie <smcv@debian.org>

* Introduce NORETURN_PTR, a version of NORETURN for function pointers

This expands to the same thing as NORETURN on gcc (and clang), but
expands to nothing on MSVC.

Signed-off-by: Simon McVittie <smcv@debian.org>

* Define NORETURN, NORETURN_PTR for non-gcc non-MSVC compilers

Signed-off-by: Simon McVittie <smcv@debian.org>
eternalcodes pushed a commit to eternalcodes/EternalJK that referenced this pull request Aug 21, 2018
* Consistently declare Com_Error implementations as noreturn

According to comments in code/game/g_public.h (whose version was
already tagged noreturn), for function pointers this only works with
the gcc/clang-specific spelling __attribute__((noreturn)), and not with
the gcc/clang/MSVC abstraction NORETURN. For function implementations,
we can use NORETURN to get MSVC support too.

This lets the compiler reason about what returns and what doesn't,
and in particular silences gcc warnings about functions that
are declared NORETURN but that did appear to return. They didn't,
because they ended with a call to (a function pointer that
eventually calls) Com_Error, but without these annotations
the compiler couldn't know that.

Signed-off-by: Simon McVittie <smcv@debian.org>

* AI_ClosestGroupEntityNumToPoint: Remove, unused

It issues compiler warnings (or did until recently), and git grep says
nothing actually calls or mentions this function.

Signed-off-by: Simon McVittie <smcv@debian.org>

* Icarus: Squash a compiler warning about possibly uninitialized length

clang can see that BufferRead() does not always assign to length,
issuing this warning:

IcarusImplementation.cpp|601 col 13| warning: ‘length’ may be used uninitialized in this function [-Wmaybe-uninitialized]

(This only happens if the buffer is null, in which case we have
bigger problems.)

Signed-off-by: Simon McVittie <smcv@debian.org>

* Move definition of NORETURN to q_platform.h

Signed-off-by: Simon McVittie <smcv@debian.org>

* Introduce NORETURN_PTR, a version of NORETURN for function pointers

This expands to the same thing as NORETURN on gcc (and clang), but
expands to nothing on MSVC.

Signed-off-by: Simon McVittie <smcv@debian.org>

* Define NORETURN, NORETURN_PTR for non-gcc non-MSVC compilers

Signed-off-by: Simon McVittie <smcv@debian.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants