_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Denis,
> Oof, ouch.
>
> In my defense, I followed the advice given in Linux CodingStyle document.
> Most of the indentation issues you had a problem with was due to me using
> 'indent' with the suggested options.
>
> In the future I will happily follow the code style conventions you outline.
> However, given that this is my first serious submission I cannot be expected
> to 'guess' the conventions if they are not written down, especially since
> these are not outlined in the Linux CodingStyle document.
I simply have to keep the coding style unique for the whole project,
because otherwise patching will become a nightmare. We actually follow
the Linux kernel coding style and the best documentation is the paper
from Greg KH a couple of years ago. I linked the paper and his talk from
this page:
http://www.bluez.org/development.html
And one small piece of advise. Don't use indent. Nothing good comes out
of it. Following the kernel coding style also implies to rethink the
usage of goto and some other constructs where universities tried to tell
you that these are bad. However this is a learning process and when you
submit more and more patch you will definitely get a feel for it.
> Since it seems that BlueZ has further conventions that developers should be
> following, which are not outlined in the Linux Coding Style documents, I
> suggest that you include them on the Coding Style section of the Development
> page of bluez.org to avoid such problems in the future :)
I should write something about the extra conventions, but to my shame, I
am too lazy.
Regards
Marcel
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel,
Oof, ouch.
In my defense, I followed the advice given in Linux CodingStyle document.
Most of the indentation issues you had a problem with was due to me using
'indent' with the suggested options.
In the future I will happily follow the code style conventions you outline.
However, given that this is my first serious submission I cannot be expected
to 'guess' the conventions if they are not written down, especially since
these are not outlined in the Linux CodingStyle document.
Since it seems that BlueZ has further conventions that developers should be
following, which are not outlined in the Linux Coding Style documents, I
suggest that you include them on the Coding Style section of the Development
page of bluez.org to avoid such problems in the future :)
Regards,
-Denis
> You are doing way too much casts. Especially all casts for malloc() and
> free() are totally unneeded. Make sure you include malloc.h and
> everything will be fine. Check the manual pages for needed includes if
> you are unsure. Every cast has a potential to hide an error. Please do
> only casts if you really have to and you are 100% sure it is the only
> way to make this code working.
>
> Pointer are never initialized with "= 0". Never. If you have to, then
> it is "= NULL". The same applies to any comparison and I prefer doing
> this with "!" as NULL check.
>
> In general, the assigned of a variable when declaring it will only hide
> programming mistakes that a compiler warning might have found. This
> applies to any compiler warning you see. Don't try to hide. Understand
> the real cause for it and fix that.
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Denis,
> Here's a fixed up version of the patch.
not it is not. I spent the last hours to fix your coding style mistakes.
I really like the stuff you do, but the more time I have to spent to fix
your code, the unlikelier I might include it. Here are some tips:
You are doing way too much casts. Especially all casts for malloc() and
free() are totally unneeded. Make sure you include malloc.h and
everything will be fine. Check the manual pages for needed includes if
you are unsure. Every cast has a potential to hide an error. Please do
only casts if you really have to and you are 100% sure it is the only
way to make this code working.
Pointer are never initialized with "= 0". Never. If you have to, then
it is "= NULL". The same applies to any comparison and I prefer doing
this with "!" as NULL check.
In general, the assigned of a variable when declaring it will only hide
programming mistakes that a compiler warning might have found. This
applies to any compiler warning you see. Don't try to hide. Understand
the real cause for it and fix that.
No public function when they are not needed. I prefer you declare
everything as static first and then non-static if needed. In this case
the compiler also warns you about unused code.
Keep the API simple. All of this is internal stuff and we can change it
without breaking any other applications. Having only one function for
the XML parsing makes it really simple and you don't have to play any
nasty typedef tricks. Speaking of which, typedefs should be avoided
whenever possible. Our SDP API is not a good example for that, I know
that, but it is too late to change it.
> I've made a script that dumps all profiles that sdptool supports and registers
> them back using XML and compares the results. It seems to work for all of
> them, but further testing would be appreciated.
Care to send a patch for sdptool that retrieves SDP records in XML
format.
Regards
Marcel
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Denis,
> > I've modified the service-agent example to take a new argument, --xmlfile,
> > which will attempt to register a record stored in an XML format.
>
> Nice idea. I like that. The dbus_message_append_args() can actually add
> more then one argument at a time, this is why it is called "args". And
> please use mmap() for reading the XML file.
the code is untested, but I committed a version to the CVS that actually
implements your idea.
Regards
Marcel
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Denis,
> Attached is a patch that adds an expat based parser for parsing XML records.
> I've also added a new API function AddServiceRecordAsXML (does the same thing
> as AddServiceRecord, but in XML format) to the Manager hierarchy.
looks good to me, but please follow the coding style of BlueZ. In your
you have way to many whitespace between function names and pointers etc.
> I've modified the service-agent example to take a new argument, --xmlfile,
> which will attempt to register a record stored in an XML format.
Nice idea. I like that. The dbus_message_append_args() can actually add
more then one argument at a time, this is why it is called "args". And
please use mmap() for reading the XML file.
Regards
Marcel
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel