Here are some bug fix and corrections patches.
This is my first attempt, so please forgive errors and let me know if there is any problem.
B.
Two more bugfix patches. Please give me feedback if these patches are working ok.
B.
On Fri, May 17, 2013 at 1:08 PM, Bhaskar11 niceguy108@gmail.com wrote:
Here are some bug fix and corrections patches.
This is my first attempt, so please forgive errors and let me know if there is any problem.
B.
On Sat, May 18, 2013 at 11:13 AM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Sat, May 18, 2013 at 10:59:25AM +0530, Bhaskar11 wrote:
Two more bugfix patches. Please give me feedback if these patches are working ok.
thanks, i will attempt to review/merge them over the next couple of days.
I had a quick look at the last two, but I'm not convinced, I'll probably rewrite a fix for those.
1) There is actually 3 places where trans_assign_trans_id is used and the one that's correct uses 'int' and not 'int8_t' and this seems a better approach because that's the native return type of the trans_assign_trans_id function. This way we have 'int' (return value allowing full valid range of uint8_t + negative values for error) and 'uint8_t' ( final valid value stored in structure ), and we avoid going from int -> int8_t -> uint8_t. And as a bonus the three places in the code with this pattern are now consistent with each other.
2) The patch title is wrong : - It actually says "should be unsigned int to allow negative values" - It's also too long and doesn't include an indication of the subsystem it refers to.
3) IMHO, Those two patches are trivial and similar enough to have been made in one commit.
I've attached as an example the corrected patch I will apply, for future reference.
I'll probably get to the other later in the day.
Cheers,
Sylvain
Three few more patches. Hope these are better done.
B.
On Sat, May 18, 2013 at 3:07 PM, Sylvain Munaut 246tnt@gmail.com wrote:
On Sat, May 18, 2013 at 11:13 AM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Sat, May 18, 2013 at 10:59:25AM +0530, Bhaskar11 wrote:
Two more bugfix patches. Please give me feedback if these patches are working ok.
thanks, i will attempt to review/merge them over the next couple of days.
I had a quick look at the last two, but I'm not convinced, I'll probably rewrite a fix for those.
- There is actually 3 places where trans_assign_trans_id is used and
the one that's correct uses 'int' and not 'int8_t' and this seems a better approach because that's the native return type of the trans_assign_trans_id function. This way we have 'int' (return value allowing full valid range of uint8_t + negative values for error) and 'uint8_t' ( final valid value stored in structure ), and we avoid going from int -> int8_t -> uint8_t. And as a bonus the three places in the code with this pattern are now consistent with each other.
- The patch title is wrong :
- It actually says "should be unsigned int to allow negative values"
- It's also too long and doesn't include an indication of the
subsystem it refers to.
- IMHO, Those two patches are trivial and similar enough to have been
made in one commit.
I've attached as an example the corrected patch I will apply, for future reference.
I'll probably get to the other later in the day.
Cheers,
Sylvain
Hi Bhaskar,
On Fri, May 17, 2013 at 01:08:53PM +0530, Bhaskar11 wrote:
Here are some bug fix and corrections patches.
I've applied all patches of this [first] set, thanks for your contributions.
This is my first attempt, so please forgive errors and let me know if there is any problem.
Recommendations for the future: * please follow the rules regarding the commit message i've written in my other mail
* please send every patch as a separate mail to this list, preferrably by using 'git send-email'. This way we don't have to manually save each and every attachment but it fits the automatic workflow. Also, feedback for individual patches on the mailing list is simplified.
Regards, Harald
baseband-devel@lists.osmocom.org
-
Bhaskar11 -
Harald Welte -
Holger Hans Peter Freyther -
Sylvain Munaut