Git Product home page Git Product logo

Comments (4)

GoogleCodeExporter avatar GoogleCodeExporter commented on August 15, 2024
You don't describe what options you gave to configure, so I'm guessing either 
that
you passed in your own -W flags, or that RHEL has patched gcc to default various
warnings to 'on', that are not on by default in gcc.  In any case, in an
out-of-the-box (well, tarfile) install, perftools compiles without any warnings 
on
every Linux distribution I've tried (about 7).

I've been able to reproduce many of these warnings by adding -Wall -Wextra to 
the
configuration options.  Thank you for pointing out the flaw in our 
distribution, that
we do not enable any warning flags by default.  I will fix that for the next 
release.

As for the warnings with -Wall -Wextra, I've briefly looked through them, and 
am glad
to report none indicate a real problem (as far as I can tell).  The two biggest
category of warnings -- signed/unsigned and unused parameters -- are due to 
explicit
coding standards we have.  For instance, we consider documenting parameters, 
even
unused ones, to be a feature rather than a bug.  (As for signedness, signed-type
promotion is one of the biggest warts in the design of C, in my opinion, but 
I'll
spare this bug report the rant.)

As for the rest of the warnings, most are for ptmalloc, which is third-party 
code we
include in the distribution for allow for comparative tests, but which we did 
not
write.  We have no plans to modify this third-party code.  When fixing up the
perftools Makefile, I'll suppress all warnings for those ptmalloc.

The only warning left after all that is this one:

src/heap-profile-table.cc:297: warning: format '%08llx' expects type 'long long
unsigned int', but argument 4 has type 'uintptr_t'

The code actually uses "%08"PRIxPTR, which is the correct formatting code to 
use for
uintptr_t, so I have to conclude this is a buglet in the compiler.  I'll look 
into
this more, and forward the report to the gcc folks if appropriate.

Original comment by [email protected] on 22 Jul 2007 at 8:19

  • Changed state: Accepted

from gperftools.

GoogleCodeExporter avatar GoogleCodeExporter commented on August 15, 2024
You're correct, I used -Wall -Wextra with the standard GNU toolchain.

My compiling the code this way is due to my own coding standards.  I won't adopt
any third-party library without first examining its documentation and compiling 
it
to see how bad the warnings and errors are.  Both of those say a lot about how
much I would struggle to use the code, if adopted, because they say a lot about
how much care was taken to create the code.

On the issue of unsigned parameters, I think you should change your 
understanding
of your own coding standard.  "Documenting parameters, even unused ones" should 
not
be interpreted as "we don't care about compiler warnings".  Writing clean code 
is
not inconsistent with getting clean compiles.  For instance, the following code 
in
src/base/googleinit.h:

     39   GoogleInitializer(const char* name, void_function f) {
     40     f();
     41   }

generates this warning:

    ./src/base/googleinit.h:39: warning: unused parameter 'name'

But this can be easily fixed in a manner that continues to document the unused
parameter:

     39   GoogleInitializer(const char* /* name */, void_function f) {
     40     f();
     41   }

and this alternate form does not generate any warnings.

Why do I harp so much on writing code so clean it generates *no* warnings?
It's because if you get into the habit of allowing warnings, then you will
also get into the habit of ignoring them when they show up.  And then all
that good advice the compiler gave you about important problems will be
overlooked, lost in the noise of unimportant problems, and you'll spend
hours and days trying to track down mysterious behaviors when you already
had automation in place that had pointed out the problem.

In my own code, I have my post-build log-file analyzer run immediately after
every nightly build, and the results are emailed to me.  With clean code
to start with, it's easy to see if any problems have been created, and to
address them quickly each day.  In contrast, if you already have 120 of
some type of warning in each night's build, who will bother to go track
down some new ones that show up, or even notice that the count has changed
in the first place?

So on issues, say, of signedness/unsignedness, stop blaming the language.
It is what it is.  If it demands that you state your assumptions clearly
to avoid warnings, say by putting appropriate casts in place, then do so.
(The assumption in a signed/unsigned comparison is that you won't be
comparing against negative values.  An explicit cast says to both humans
and the compiler that you thought about it and are willing to guarantee
this contract.)  This is a one-time cost for each piece of code you clean
up, and once you have the habit, the effort will be repaid many times by
preventing long debug sessions later on.

In light of that, please don't suppress warnings for third-party libraries
you incorporate.  It's important for everybody to see potential problems,
not to hide them.  Instead, push the upstream provider to distribute clean
code themselves.

On the issue of the pointer format, we have this on our 32-bit platform:

/usr/include/bits/wordsize.h:

    #define __WORDSIZE      32

/usr/include/inttypes.h:

    # if __WORDSIZE == 64
    #  define __PRI64_PREFIX        "l"
    #  define __PRIPTR_PREFIX       "l"
    # else
    #  define __PRI64_PREFIX        "ll"
    #  define __PRIPTR_PREFIX
    # endif

    # define PRIxPTR        __PRIPTR_PREFIX "x"

so in this code:

    296     printed = snprintf(buf + buflen, bufsize - buflen, " 0x%08" PRIxPTR,
    297                        reinterpret_cast<uintptr_t>(b.stack[d]));

I don't see where the compiler is getting its idea:

    src/heap-profile-table.cc:297: warning: long long unsigned int format, different
type arg (arg 4)

that the format contains "ll".

    % gcc --version
    gcc (GCC) 3.4.6 20060404 (Red Hat 3.4.6-8)

-----------------

I'm also seeing this outright error when I try to compile the library with
the compiler's -g option instead of -O3, to include all the debug symbols:

    if gcc -DHAVE_CONFIG_H -I. -I. -I./src  -I./src -D_POSIX_C_SOURCE=200112L
-D_BSD_SOURCE -I/build/thirdparty/include -I/build/my_application/include  
-DUSE_P
THREADS -g -Wall -Wextra -pthread -MT ptmalloc_unittest2-t-test2.o -MD -MP -MF
".deps/ptmalloc_unittest2-t-test2.Tpo" -c -o ptmalloc_unittest2-t-test2.o `test 
-
f 'src/tests/ptmalloc/t-test2.c' || echo './'`src/tests/ptmalloc/t-test2.c; \
    then mv -f ".deps/ptmalloc_unittest2-t-test2.Tpo"
".deps/ptmalloc_unittest2-t-test2.Po"; else rm -f
".deps/ptmalloc_unittest2-t-test2.Tpo"; exit 1; fi
    src/tests/ptmalloc/malloc-machine.h: In function `mutex_unlock':
    src/tests/ptmalloc/malloc-machine.h:81: warning: matching constraint does not
allow a register
    src/tests/ptmalloc/malloc-machine.h:81: warning: matching constraint does not
allow a register
    src/tests/ptmalloc/malloc-machine.h:81: error: inconsistent operand constraints
in an `asm'

Original comment by [email protected] on 22 Jul 2007 at 10:03

from gperftools.

GoogleCodeExporter avatar GoogleCodeExporter commented on August 15, 2024
} Why do I harp so much on writing code so clean it generates *no* warnings?

I imagine you'll find many more warnings if you use more warnings flags!  For
instance, I have no idea what would happen if you tried to compile perftools 
with
-pedantic.  The next release of perftools will default to using a set of warning
flags that we think is appropriate for perftools.

I respect that you may have a different idea of what warnings are appropriate 
to turn
on for your own software projects.  However, we're comfortable with the list of
warnings that we enable.

} I don't see where the compiler is getting its idea:
}
}   src/heap-profile-table.cc:297: warning: long long unsigned int format, 
different
type arg (arg 4)
}
} that the format contains "ll".

It's not taking the definition in inttypes.h, unfortunately, because a necessary
preprocessor #define isn't being made early enough.  This will be fixed in the 
next
release.

}     src/tests/ptmalloc/malloc-machine.h: In function `mutex_unlock':

Yes, this seems to be a bug with the ptmalloc code.  If you do not wish to test
tcmalloc against ptmalloc (which is the malloc used by glibc), you can ignore, 
or
comment out, builds of this file.

Original comment by [email protected] on 22 Jul 2007 at 11:23

from gperftools.

GoogleCodeExporter avatar GoogleCodeExporter commented on August 15, 2024
The ptmalloc code continued to give problems (other than the ones discussed 
here), so
I ended up taking them out of the default build.

I also set up the Makefile to use the same -W flags that we used internally, 
and got
rid of warning messages on all platforms that we test on (see INSTALL file for a
list).  There are still warnings with the -W settings that you mention above, 
but I
think those are unlikely to get fixed, so I'm going to close out this bug.

Original comment by [email protected] on 17 Aug 2007 at 9:32

  • Changed state: Fixed

from gperftools.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.