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 compile warnings #590

Merged
merged 9 commits into from Dec 29, 2018
Merged

Fix compile warnings #590

merged 9 commits into from Dec 29, 2018

Conversation

@fedux
Copy link
Contributor

@fedux fedux commented Dec 21, 2018

This fixes many compile warnings produced when compiling the code with -Wall. For the explanation and purpose of the warning see the compiler man page.

fedux added 8 commits Dec 21, 2018
Use macros defined in inttypes.h for the proper format for 64 bit types
and fall-back to previously define format when inttypes.h is not
available.
Don't define variables only used for debuggin when debug is not enabled.
Declare variables in the right order to avoid this warning.
Rewrite the intentional segafult to avoid the compile warning.
Initialize codepoint to avoid the warning.
Use value-initialization instead of clearing an object with memset.
Fix sign compare warning by improving casting choices.
@hugbug
Copy link
Member

@hugbug hugbug commented Dec 26, 2018

Thanks for your contribution. A description would be helpful.

From all the changes I think I'm going to accept these two:

  • sign-compare;
  • unused-variable.

For other changes:

  • format - makes code unnecessary uglier;
  • reorder - I don't know what the purposes of changes were (a description of your intent would be helpful);
  • nonnull - fixes the warning for you but produces a warning in other compilers (dereference of null pointer);
  • maybe-uninitialized - false diagnostic;
  • misleading-indentation and class-memaccess - code in third party libraries. I'd rather not change it;
  • stringop-truncation/stringop-overflow - introduces usage of strcpy which other compilers consider dangerous.

@fedux
Copy link
Contributor Author

@fedux fedux commented Dec 26, 2018

Hi @hugbug , for the reason of each of the fixes you can check the man page of gcc for example for a detailed explanation.

For these other changes:

  • format - makes code unnecessary uglier;
    The reason is that to support both 32bits and 64bits platforms the proper format for printf and similar depends on the length of the word and thus it needs to be defined in macros.

  • reorder - I don't know what the purposes of changes were (a description of your intent would be helpful);
    From the man page:

       -Wreorder (C++ and Objective-C++ only)
           Warn when the order of member initializers given in the code does not match the order in which they must be executed.  For instance:

                   struct A {
                     int i;
                     int j;
                     A(): j (0), i (1) { }
                   };

           The compiler rearranges the member initializers for "i" and "j" to match the declaration order of the members, emitting a warning to that effect.
  • nonnull - fixes the warning for you but produces a warning in other compilers (dereference of null pointer);
    Ok, which compiler? Did it produce a warning before?

  • maybe-uninitialized - false diagnostic;
    Are you sure? This is the context:

    unsigned int codepoint;
    while (*utfstr != 0)
    {
        unsigned char ch = (unsigned char)*utfstr;
        if (ch <= 0x7f)
            codepoint = ch;
        else if (ch <= 0xbf)
            codepoint = (codepoint << 6) | (ch & 0x3f);
        else if (ch <= 0xdf)
            codepoint = ch & 0x1f;

, if ch doesn't match the first if but matches the first else if then codepoint will be used uninitialized.

  • misleading-indentation and class-memaccess - code in third party libraries. I'd rather not change it;
    OK.

  • stringop-truncation/stringop-overflow - introduces usage of strcpy which other compilers consider dangerous.
    Then problem is that the way the original strncpy is used is also used in an unsafe way because it checks the length of the source buffer instead of the destination buffer causing a posible overflow. Using strcpy instead doesn't make it any worse as it has the same effect as the current wrong use of strncpy and the destination buffer is already pre-allocated to the right size a few lines above. I could split this commit in two to address both warnings separately.

The whole purpose of these fixes is to be able to compile with -Wall -Werror and enforce better code quality.

@hugbug
Copy link
Member

@hugbug hugbug commented Dec 26, 2018

maybe-uninitialized - false diagnostic;
Are you sure? This is the context:

You are right. That's a bug. Good thing to have this warning enabled then.

nonnull - fixes the warning for you but produces a warning in other compilers (dereference of null pointer);
Ok, which compiler? Did it produce a warning before?

That code is 10 years old, I don't remember what it was back then. Right now Clang produces warning when compiling in analyse mode. Not a big issue probably if the purpose is -Wall. I can accept the change.

stringop-truncation/stringop-overflow

Can you change the code somehow to not use strcpy?

You convinced me, I'm going to accept the whole PR. Although I don't like some changes they are probably for good.

@fedux
Copy link
Contributor Author

@fedux fedux commented Dec 27, 2018

Can you change the code somehow to not use strcpy?

Changed. If you think there is something else in this PR that could be improved let me know.

@hugbug hugbug merged commit 31a34b5 into nzbget:develop Dec 29, 2018
1 check passed
@hugbug
Copy link
Member

@hugbug hugbug commented Dec 29, 2018

Thank you.

hugbug added a commit that referenced this issue Jan 3, 2019
when cross-compiling: PRId64 and others maybe undefined even if
<inttypes.h> exists
hugbug added a commit that referenced this issue Jan 6, 2019
hugbug added a commit that referenced this issue Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants