Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Android Project Changeset 4f8b683: libc/memset.c

review.source.android.com

Post reply on HN
Print view View original

31–40 of 93 posts

Re: Android Project Changeset 4f8b683: libc/memset.c

#31

Post by hoelle »

pkaler wrote: post #17

Forget the typo. This is the slowest, most naive implementation of memset possible. Is this actual shipping code? I've followed the changeset up to the top and it looks like it's in platform/bootable/bootloader/legacy. Hopefully this isn't actually called anywhere, but the review date says May 13th. What's the point of optimizing the Java VM when functions like memset, malloc, memcmp, memcpy, memchr, etc, etc haven't…

I'll offer the tip that even if you have a fast memset, you can and should avoid using it when possible.

Seeing memset is usually a sign that someone is being lazy about watching the length of their buffers and/or verifying their copies aren't off-by-one.

Re: Android Project Changeset 4f8b683: libc/memset.c

#32

Post by humbledrone »

neilc wrote: post #24
humbledrone wrote: post #18

Earlier quoted context omitted.

Huh? Any halfway decent software shop will have a policy of compiling with all warnings enabled, and fixing all sources of warnings (with only very rare exceptions). It is extremely unprofessional to disable or ignore warnings such as the "unused function parameter" warning. If you disagree, I respectfully submit that I would never let you near a C project that I was working on.

Any halfway decent software shop will have a policy of compiling with all warnings enabled FWIW, your definition of "halfway decent" would exclude most software shops in the world. It is extremely unprofessional to disable or ignore warnings such as the "unused function parameter" warning Unused function parameter warnings are often noise (e.g., due to #ifdefs, unimplemented APIs, backward compatible APIs, etc). I ag…

I would probably agree that most software shops in the world are not halfway decent.

The benefits of tracking down and squelching such warnings would have very likely caused this bug to never exist. The point is that you make "compiles without warnings" a policy, and you build a habit of maintaining that policy -- after a while it begins to take almost no effort at all.

Re: Android Project Changeset 4f8b683: libc/memset.c

#33

Post by ars »

pkaler wrote: post #17

Forget the typo. This is the slowest, most naive implementation of memset possible. Is this actual shipping code? I've followed the changeset up to the top and it looks like it's in platform/bootable/bootloader/legacy. Hopefully this isn't actually called anywhere, but the review date says May 13th. What's the point of optimizing the Java VM when functions like memset, malloc, memcmp, memcpy, memchr, etc, etc haven't…

How would you write this function? Fully portable, so no ASM.

Re: Android Project Changeset 4f8b683: libc/memset.c

#34

Post by tptacek »

humbledrone wrote: post #20
DrSprout wrote: post #16

Earlier quoted context omitted.

You have to know that -Wextra exists, which I for one didn't until just now when I ran a sample program. All my projects just compiled with -Wall, which doesn't catch that error. Also, unless every programmer on the project is being scrupulous about finding and correcting such errors, it quickly becomes line noise, and a lot of it completely unimportant stuff.

Yeah, that's why every programmer on the project has to be scrupulous about finding and correcting such errors. That's professionalism 101 -- if your team is not keeping the build warning free, things like this slip through.

Things like what, a blatantly obvious bug that would crash virtually every C program running on the system were it not for the fact that it was dead code?

This is practically a documentation bug.

[edit]

I kind of take it back, after mahmud's bzero() joke... out of oniguruma, Python, hexfiend, Apache, nginx, openssl, redis, regexkit, saxon, memcached, subversion, and valgrind, this hypothetical bug would have broken Python, redis, subversion, several apache modules, and memcached. The contents of my codebase/3p directory, FWIW.

Re: Android Project Changeset 4f8b683: libc/memset.c

#35

Post by tptacek »

humbledrone wrote: post #32
neilc wrote: post #24

Earlier quoted context omitted.

Any halfway decent software shop will have a policy of compiling with all warnings enabled FWIW, your definition of "halfway decent" would exclude most software shops in the world. It is extremely unprofessional to disable or ignore warnings such as the "unused function parameter" warning Unused function parameter warnings are often noise (e.g., due to #ifdefs, unimplemented APIs, backward compatible APIs, etc). I ag…

I would probably agree that most software shops in the world are not halfway decent. The benefits of tracking down and squelching such warnings would have very likely caused this bug to never exist. The point is that you make "compiles without warnings" a policy, and you build a habit of maintaining that policy -- after a while it begins to take almost no effort at all.

Do you write C code?

Re: Android Project Changeset 4f8b683: libc/memset.c

#36

Post by humbledrone »

jmillikin wrote: post #28
humbledrone wrote: post #9

The thing that shocks me about this bug is that it should have been caught when the compiler issued a warning about an unused function parameter. Either the warning was not enabled, or the programmer just ignored it; either of those options indicates a pretty severe lack of professionalism.

The "unused parameter" error is one of the most useless of C compiler errors. There are dozens of perfectly valid reasons why a parameter would go unused, so the occasional error just gets lost in the noise. It's more helpful in C++, which can omit parameter names to indicate that it's intentionally unused. But then you're using C++; not worth the tradeoff.

It's not hard to type a statement like "(void)v;" to explicitly say, "v is intentionally unused." I fail to see the drawback of this approach. Of course you should only do this when (1) you can't remove the parameter from the function's prototype and (2) there is a good reason for it to be unused. But once you develop that habit, the compiler can help you catch bugs that are otherwise easy to miss. Sounds like a worthwhile tradeoff to me.

Re: Android Project Changeset 4f8b683: libc/memset.c

#37

Post by tptacek »

queensnake wrote: post #29
klodolph wrote: post #22

Earlier quoted context omitted.

"Pretty severe" lack of professionalism? It's not unprofessional not to turn the warnings all the way up and sift through each and every one. Some of us work code inherited from other people with different coding guidelines, and some of the warnings will generate loads of messages for code that is not only technically correct but stylistically correct as well. The unused parameter warning is one of the biggest offend…

| The unused parameter warning is one of the biggest offenders in this area. And if you have a hundred warnings for unused parameters, which ones do you pay attention to? Well, so then you're exposed to this error. Do: void foo(int x) { (void)x; } to silence unused parameter warnings.

Go look at Google code search, which includes vast quantities of code written by professional teams and (often later) open sourced. Very little of it casts expressions to (void). When I started at Arbor, I got rid of all the (void) casts. What an eyesore.

Very little C code is const-correct either. Most of the C devs I know make fun of const-correctness.

Re: Android Project Changeset 4f8b683: libc/memset.c

#38

Post by tptacek »

ars wrote: post #33
pkaler wrote: post #17

Forget the typo. This is the slowest, most naive implementation of memset possible. Is this actual shipping code? I've followed the changeset up to the top and it looks like it's in platform/bootable/bootloader/legacy. Hopefully this isn't actually called anywhere, but the review date says May 13th. What's the point of optimizing the Java VM when functions like memset, malloc, memcmp, memcpy, memchr, etc, etc haven't…

How would you write this function? Fully portable, so no ASM.

You'd unroll the loop. But knowing the average workload for memset(), it probably wouldn't be a perceptible win.

Re: Android Project Changeset 4f8b683: libc/memset.c

#39

Post by earl »

klodolph wrote: post #22
humbledrone wrote: post #9

The thing that shocks me about this bug is that it should have been caught when the compiler issued a warning about an unused function parameter. Either the warning was not enabled, or the programmer just ignored it; either of those options indicates a pretty severe lack of professionalism.

"Pretty severe" lack of professionalism? It's not unprofessional not to turn the warnings all the way up and sift through each and every one. Some of us work code inherited from other people with different coding guidelines, and some of the warnings will generate loads of messages for code that is not only technically correct but stylistically correct as well. The unused parameter warning is one of the biggest offend…

Current codebase: 9734 warnings.

As a point of pride, none of them were created by me, but it is what it is. Some of them come from since deprecated functions and nobody wants to go back and mess with old code; some of them come from sloppiness; some of them come from java's collection handling.

And yet this codebase gets a lot of work accomplished anyway.

Re: Android Project Changeset 4f8b683: libc/memset.c

#40

Post by seliopou »

DrSprout wrote: post #16
humbledrone wrote: post #9

The thing that shocks me about this bug is that it should have been caught when the compiler issued a warning about an unused function parameter. Either the warning was not enabled, or the programmer just ignored it; either of those options indicates a pretty severe lack of professionalism.

You have to know that -Wextra exists, which I for one didn't until just now when I ran a sample program. All my projects just compiled with -Wall, which doesn't catch that error. Also, unless every programmer on the project is being scrupulous about finding and correcting such errors, it quickly becomes line noise, and a lot of it completely unimportant stuff.

-Wextra is total insanity. I just compiled some C code I've been working on with that flag, and got the following: "warning: signed and unsigned type in conditional expression." I was testing if a variable I declared as an integer was less than 0. To my knowledge, every number literal in C is assumed to be of a signed type unless followed by a 'u'. But now I know better. 0 is the exception.
Post reply on HN