* I'd like to contribute but I don't know how... @ 2018-02-22 2:33 Alex Arvelaez 2018-02-22 3:20 ` valdis.kletnieks at vt.edu 0 siblings, 1 reply; 6+ messages in thread From: Alex Arvelaez @ 2018-02-22 2:33 UTC (permalink / raw) To: kernelnewbies Hello, I'd like to contribute to the linux kernel eventually but I'm not sure how, I grabbed a copy of the source code and I found a FIXME that looks like I could fix: /* File: /usr/src/linux/tools/perf/util/string.c * FIXME: replace this with an expression using log10() when we * find a suitable implementation, maybe the one in the dvb drivers... */ I found the implementation they mention, is it okay to just copy paste it into the file? I'm not sure where else to ask this kind of question... Regards, Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how... 2018-02-22 2:33 I'd like to contribute but I don't know how Alex Arvelaez @ 2018-02-22 3:20 ` valdis.kletnieks at vt.edu 2018-02-22 3:46 ` Jeffrey Walton 2018-02-22 4:12 ` Alex Arvelaez 0 siblings, 2 replies; 6+ messages in thread From: valdis.kletnieks at vt.edu @ 2018-02-22 3:20 UTC (permalink / raw) To: kernelnewbies On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said: > Hello, > > I'd like to contribute to the linux kernel eventually but I'm not sure https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.html > how, I grabbed a copy of the source code and I found a FIXME that looks > like I could fix: > > /* File: /usr/src/linux/tools/perf/util/string.c > * FIXME: replace this with an expression using log10() when we > * find a suitable implementation, maybe the one in the dvb drivers... > */ Step 0: Verify that the comment still matches the code, *and* that the change is still desired. Hint: Why do they want log10()? What does the current code do? What, if anything, will break if you change it? Do you understand *why* '28' was used? And why they didn't just go ahead and use the perfectly usable log10() found in -lm ? For bonus points, understand the code's behavior (or misbehavior) on 32 versus 64 bit architectures, and whether or not that's actually a problem. For extra bonus points, figure out how long that FIXME has been there. And why. (Note that some of these are things that I don't know the answer to offhand. ;) > I found the implementation they mention, is it okay to just copy paste > it into the file? I'm not sure where else to ask this kind of question... If it's going to be used in more than one place, it should be refactored into a function, and both usage sites reworked to call the function rather than inline. -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 486 bytes Desc: not available URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180221/0771468f/attachment-0001.sig> ^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how... 2018-02-22 3:20 ` valdis.kletnieks at vt.edu @ 2018-02-22 3:46 ` Jeffrey Walton 2018-02-22 4:12 ` Alex Arvelaez 1 sibling, 0 replies; 6+ messages in thread From: Jeffrey Walton @ 2018-02-22 3:46 UTC (permalink / raw) To: kernelnewbies On Wed, Feb 21, 2018 at 10:20 PM, <valdis.kletnieks@vt.edu> wrote: > On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said: >> ... >> >> I'd like to contribute to the linux kernel eventually but I'm not sure > > https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.html This comes up so frequently the list might consider sending a FAQ once a month with links to the answers. Its more useful than Mailman emailing plaintext passwords :) Jeff ^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how... 2018-02-22 3:20 ` valdis.kletnieks at vt.edu 2018-02-22 3:46 ` Jeffrey Walton @ 2018-02-22 4:12 ` Alex Arvelaez 2018-02-22 21:25 ` Alex Arvelaez 1 sibling, 1 reply; 6+ messages in thread From: Alex Arvelaez @ 2018-02-22 4:12 UTC (permalink / raw) To: kernelnewbies On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletnieks at vt.edu wrote: > On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said: > > Hello, > > > > I'd like to contribute to the linux kernel eventually but I'm not sure > > https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.html > > > how, I grabbed a copy of the source code and I found a FIXME that looks > > like I could fix: > > > > /* File: /usr/src/linux/tools/perf/util/string.c > > * FIXME: replace this with an expression using log10() when we > > * find a suitable implementation, maybe the one in the dvb drivers... > > */ > > Step 0: Verify that the comment still matches the code, *and* that the change > is still desired. Hint: Why do they want log10()? What does the current code > do? What, if anything, will break if you change it? It's in a function that creates boolean expressions from an array of integers and it looks they hardcoded 28 bytes per each number. I think the intent of using log10() is to only allocate as many bytes as the largest number so expressions with lower numbers(in magnitude) would use less memory. > > Do you understand *why* '28' was used? And why they didn't just go > ahead and use the perfectly usable log10() found in -lm ? I'll look into this, I think they are including stdlib.h so it's a very good point. I think they used 28 to be on the safe side 28 bytes gives room for 27 digits per number. They also put the formula they're trying to approximate: /* "%s == %d || " = log10(MAXINT) * 2 + 8 chars for the operators */ > For bonus points, understand the code's behavior (or misbehavior) on 32 versus > 64 bit architectures, and whether or not that's actually a problem. I don't think that would be a problem but I have to take a closer look at the implementation of log10(). > For extra bonus points, figure out how long that FIXME has been there. And why. The commit is from 2015, I couldn't find much about it on the lkml.org archives. According the commit message the function's used to generate to generate expressions to filter syscalls when tracing them: perf -e read,write,fork //we only track specified syscalls in this case > (Note that some of these are things that I don't know the answer to offhand. ;) > > > I found the implementation they mention, is it okay to just copy paste > > it into the file? I'm not sure where else to ask this kind of question... > > If it's going to be used in more than one place, it should be refactored > into a function, and both usage sites reworked to call the function rather > than inline. It's only a couple functions, they defined log10() in terms of log2(). I don't really know how to avoid inlining though, I'll look into it. > _______________________________________________ > Kernelnewbies mailing list > Kernelnewbies at kernelnewbies.org > https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies ^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how... 2018-02-22 4:12 ` Alex Arvelaez @ 2018-02-22 21:25 ` Alex Arvelaez 2018-02-23 23:11 ` Tobin C. Harding 0 siblings, 1 reply; 6+ messages in thread From: Alex Arvelaez @ 2018-02-22 21:25 UTC (permalink / raw) To: kernelnewbies On Thu, Feb 22, 2018 at 04:12:14AM +0000, Alex Arvelaez wrote: > On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletnieks at vt.edu wrote: > > On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said: > > > Hello, > > > > > > I'd like to contribute to the linux kernel eventually but I'm not sure > > > > https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.html > > > > > how, I grabbed a copy of the source code and I found a FIXME that looks > > > like I could fix: > > > > > > /* File: /usr/src/linux/tools/perf/util/string.c > > > * FIXME: replace this with an expression using log10() when we > > > * find a suitable implementation, maybe the one in the dvb drivers... > > > */ > > > > Step 0: Verify that the comment still matches the code, *and* that the change > > is still desired. Hint: Why do they want log10()? What does the current code > > do? What, if anything, will break if you change it? > I went ahead and used log10() from -lm, everything seems to work properly from what I can see. I made a patch with my changes should I send that to the linux-kernel mailing list for feedback? Regards, Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how... 2018-02-22 21:25 ` Alex Arvelaez @ 2018-02-23 23:11 ` Tobin C. Harding 0 siblings, 0 replies; 6+ messages in thread From: Tobin C. Harding @ 2018-02-23 23:11 UTC (permalink / raw) To: kernelnewbies On Thu, Feb 22, 2018 at 09:25:39PM +0000, Alex Arvelaez wrote: > On Thu, Feb 22, 2018 at 04:12:14AM +0000, Alex Arvelaez wrote: > > On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletnieks at vt.edu wrote: > > > On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said: > > > > Hello, > > > > > > > > I'd like to contribute to the linux kernel eventually but I'm not sure > > > > > > https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.html > > > > > > > how, I grabbed a copy of the source code and I found a FIXME that looks > > > > like I could fix: > > > > > > > > /* File: /usr/src/linux/tools/perf/util/string.c > > > > * FIXME: replace this with an expression using log10() when we > > > > * find a suitable implementation, maybe the one in the dvb drivers... > > > > */ > > > > > > Step 0: Verify that the comment still matches the code, *and* that the change > > > is still desired. Hint: Why do they want log10()? What does the current code > > > do? What, if anything, will break if you change it? > > > > I went ahead and used log10() from -lm, everything seems to work > properly from what I can see. I made a patch with my changes should > I send that to the linux-kernel mailing list for feedback? Documentation/process/ is your friend. Tobin ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-23 23:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-22 2:33 I'd like to contribute but I don't know how Alex Arvelaez 2018-02-22 3:20 ` valdis.kletnieks at vt.edu 2018-02-22 3:46 ` Jeffrey Walton 2018-02-22 4:12 ` Alex Arvelaez 2018-02-22 21:25 ` Alex Arvelaez 2018-02-23 23:11 ` Tobin C. Harding
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).