HelenOS development mailing list

Sandeep Ruhil | 3 Jun 2012 16:13

ticket #50

Hey everyone,
I am new to HelenOs and this is going to be my first open source contribution.

I want to work on a small ticket/bug to get the things started so i've chosen to work on ticket #50. I have even fixed a part of the ticket and is attaching the diff for the same (the diff is over revno 1517). I've completed the first point of ticket #50.

Please provide me feedback on whether i am doing it the right way or not. It will really help me getting started :)

--
Regards,
Sandeep Kumar

Attachment (ticket): application/octet-stream, 2953 bytes

_______________________________________________
HelenOS-devel mailing list
HelenOS-devel@...
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel

headers

Martin Decky | 9 Jun 2012 00:57

Re: ticket #50

Hello Sandeep Kumar.

>    I am new to HelenOs and this is going to be my first open source
> contribution.
>
>    I want to work on a small ticket/bug to get the things started so
> i've chosen to work on ticket #50. I  have even fixed a part of the
> ticket and is attaching the diff for the same (the diff is over revno
> 1517). I've completed the first point of ticket #50.

Thank you for your contribution and sorry for a late reply.

In essence your patch seems to do the trick, thanks again! I will try my 
best to push the functionality into our mainline branch based on your 
patch in a few days. I will not use your patch directly, though, because 
of a few nits (see below).

>    Please provide me feedback on whether i am doing it the right way or
> not. It will really help me getting started :)

You should stick more closely to the HelenOS coding style [1]. The 
formal rules are perhaps not so important than simply respecting the 
style of the existing code surrounding your modifications. For example, 
do not use C++ style comments and do not forget the space between "for" 
and the parenthesis.

Also, I would personally use more verbose variable names than "mml" and 
"mml_tmp". But this is really very subjective.

[1] http://www.helenos.org/cstyle

Best regards

Martin Decky

headers

Sandeep Ruhil | 9 Jun 2012 07:51

Re: ticket #50

Thanks Martin, i am really happy that my first contribution has made its way to the repository.

I will work on other features of the ticket in coming days and will make sure that i follow the standard coding practices.

Thanks again :)

On Sat, Jun 9, 2012 at 4:27 AM, Martin Decky <martin-Jk0gIJtyDDQ@public.gmane.org> wrote:

Hello Sandeep Kumar.

I am new to HelenOs and this is going to be my first open source
contribution.

I want to work on a small ticket/bug to get the things started so
i've chosen to work on ticket #50. I have even fixed a part of the
ticket and is attaching the diff for the same (the diff is over revno
1517). I've completed the first point of ticket #50.

Thank you for your contribution and sorry for a late reply.

In essence your patch seems to do the trick, thanks again! I will try my best to push the functionality into our mainline branch based on your patch in a few days. I will not use your patch directly, though, because of a few nits (see below).

Please provide me feedback on whether i am doing it the right way or
not. It will really help me getting started :)

You should stick more closely to the HelenOS coding style [1]. The formal rules are perhaps not so important than simply respecting the style of the existing code surrounding your modifications. For example, do not use C++ style comments and do not forget the space between "for" and the parenthesis.

Also, I would personally use more verbose variable names than "mml" and "mml_tmp". But this is really very subjective.

[1] http://www.helenos.org/cstyle

Best regards

Martin Decky

_______________________________________________
HelenOS-devel mailing list
HelenOS-devel-CzyLcWPZiU5DvUJtqsEJ4g@public.gmane.org
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel

--
Regards,
Sandeep Kumar

_______________________________________________
HelenOS-devel mailing list
HelenOS-devel@...
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel

headers

Sandeep Ruhil | 10 Jun 2012 11:46

Re: ticket #50

Hey,

I am encountering a small problem in working on second point of this ticket.

I need an object of indev_t inside the function symtab_compl of file kernel/generic/src/debug/symtab.c but when i include the header file <console/chardev.h> (containing the definition of indev_t) in the file kernel/generic/include/symtab.h (containing the prototype of symtab_compl function), i get the following compilation error :

In file included from generic/include/console/chardev.h:40:0,
         from generic/include/symtab.h:39,
         from generic/include/debug.h:39,
         from generic/include/synch/spinlock.h:42,
         from generic/include/proc/scheduler.h:38,
         from arch/ia32/src/proc/scheduler.c:35:
generic/include/synch/waitq.h:56:2: error: expected specifier-qualifier-list before 'IRQ_SPINLOCK_DECLARE'

When i inspected the reason, it turned out that :
    spinlock.h includes debug.h
    debug.h includes symtab.h
    symtab.h includes chardev.h (my include)
    chardev.h includes waitq.h
    waitq.h uses IRQ_SPINLOCK_DECLARE, which is declared in spinlock.h but spinlock.h is not fully included as the compiler is following an inclusion of spinlock.h file and hence compiler cannot find the definition of IRQ_SPINLOCK_DECLARE macro.

What can i do to resolve this error?

--
Regards,
Sandeep Kumar

_______________________________________________
HelenOS-devel mailing list
HelenOS-devel@...
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel

headers

Vojtech Horky | 29 Jun 2012 09:08

Re: ticket #50

Hello Sandeep Kumar,
sorry for replying so late. I hope you are still with us .

2012/6/10 Sandeep Ruhil <sandeep.kumar.ruhil <at> gmail.com>:
> Hey,
>
> I am encountering a small problem in working on second point of this ticket.
>
> I need an object of indev_t inside the function symtab_compl of file
> kernel/generic/src/debug/symtab.c but when i include the header file
> <console/chardev.h> (containing the definition of indev_t) in the file
> kernel/generic/include/symtab.h (containing the prototype of symtab_compl
> function), i get the following compilation error :
>
> In file included from generic/include/console/chardev.h:40:0,
>          from generic/include/symtab.h:39,
>          from generic/include/debug.h:39,
>          from generic/include/synch/spinlock.h:42,
>          from generic/include/proc/scheduler.h:38,
>          from arch/ia32/src/proc/scheduler.c:35:
> generic/include/synch/waitq.h:56:2: error: expected specifier-qualifier-list
> before 'IRQ_SPINLOCK_DECLARE'
>
> When i inspected the reason, it turned out that :
>     spinlock.h includes debug.h
>     debug.h includes symtab.h
>     symtab.h includes chardev.h (my include)
>     chardev.h includes waitq.h
>     waitq.h uses IRQ_SPINLOCK_DECLARE, which is declared in spinlock.h but
> spinlock.h is not fully included as the compiler is following an inclusion
> of spinlock.h file and hence compiler cannot find the definition of
> IRQ_SPINLOCK_DECLARE macro.
>
> What can i do to resolve this error?
I would go for splitting the symtab.h into two files. Keep the lookup
functionality in one and move printing functions (symtab_print_search
and symtab_compl) to another one.

This would not break any files except those under console/*.c so it
shall be pretty painless. The only problem is that these functions
require the symtab_search_one() function that is private (i.e. static)
to symtab.c. But I think that it is okay to make this function public
and move it to symtab.h.

Hope this helps.

Regards,
- Vojta

>
>
> --
> Regards,
> Sandeep Kumar
>
>
> _______________________________________________
> HelenOS-devel mailing list
> HelenOS-devel <at> lists.modry.cz
> http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
>

_______________________________________________
HelenOS-devel mailing list
HelenOS-devel <at> lists.modry.cz
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel

headers

Sandeep Ruhil | 4 Jul 2012 20:34

Re: ticket #50

Thanks Vojteck,
   i have solved the problem by splitting symtab.h, as per your suggestion.
I am attaching a full patch for this ticket, it is over revision no.
1534. Please
review it and let me know if there is any space for improvement :)

On Fri, Jun 29, 2012 at 12:38 PM, Vojtech Horky
<vojtech.horky@...> wrote:
>
> Hello Sandeep Kumar,
> sorry for replying so late. I hope you are still with us .
>
> 2012/6/10 Sandeep Ruhil <sandeep.kumar.ruhil@...>:
> > Hey,
> >
> > I am encountering a small problem in working on second point of this ticket.
> >
> > I need an object of indev_t inside the function symtab_compl of file
> > kernel/generic/src/debug/symtab.c but when i include the header file
> > <console/chardev.h> (containing the definition of indev_t) in the file
> > kernel/generic/include/symtab.h (containing the prototype of symtab_compl
> > function), i get the following compilation error :
> >
> > In file included from generic/include/console/chardev.h:40:0,
> >          from generic/include/symtab.h:39,
> >          from generic/include/debug.h:39,
> >          from generic/include/synch/spinlock.h:42,
> >          from generic/include/proc/scheduler.h:38,
> >          from arch/ia32/src/proc/scheduler.c:35:
> > generic/include/synch/waitq.h:56:2: error: expected specifier-qualifier-list
> > before 'IRQ_SPINLOCK_DECLARE'
> >
> > When i inspected the reason, it turned out that :
> >     spinlock.h includes debug.h
> >     debug.h includes symtab.h
> >     symtab.h includes chardev.h (my include)
> >     chardev.h includes waitq.h
> >     waitq.h uses IRQ_SPINLOCK_DECLARE, which is declared in spinlock.h but
> > spinlock.h is not fully included as the compiler is following an inclusion
> > of spinlock.h file and hence compiler cannot find the definition of
> > IRQ_SPINLOCK_DECLARE macro.
> >
> > What can i do to resolve this error?
> I would go for splitting the symtab.h into two files. Keep the lookup
> functionality in one and move printing functions (symtab_print_search
> and symtab_compl) to another one.
>
> This would not break any files except those under console/*.c so it
> shall be pretty painless. The only problem is that these functions
> require the symtab_search_one() function that is private (i.e. static)
> to symtab.c. But I think that it is okay to make this function public
> and move it to symtab.h.
>
> Hope this helps.
>
> Regards,
> - Vojta
>
>
> >
> >
> > --
> > Regards,
> > Sandeep Kumar
> >
> >
> > _______________________________________________
> > HelenOS-devel mailing list
> > HelenOS-devel@...
> > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
> >
>
> _______________________________________________
> HelenOS-devel mailing list
> HelenOS-devel@...
> http://lists.modry.cz/cgi-bin/listinfo/helenos-devel

--
Regards,
Sandeep Kumar

Attachment (ticket50.diff): application/octet-stream, 10 KiB

_______________________________________________
HelenOS-devel mailing list
HelenOS-devel@...
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel

headers

Vojtech Horky | 10 Jul 2012 15:27

Re: ticket #50

Hello Sandeep Kumar,
thanks for the patch.

2012/7/4 Sandeep Ruhil <sandeep.kumar.ruhil@...>:
> Thanks Vojteck,
>    i have solved the problem by splitting symtab.h, as per your suggestion.
> I am attaching a full patch for this ticket, it is over revision no.
> 1534. Please
> review it and let me know if there is any space for improvement :)
Definitely the symbol names completion works and it is a joy to use
the call* commands now.

However, I have a few comments and also several ideas for even better
tab completion.

First of all, your patch does not compile for 64bit architectures due
to printf-related warning. Please, do use %zu when printing size_t
type (as this is quite common error, I created a special wiki page for
it [1]).

Next, if you look at the final patch, there are two pretty long blocks
of virtually the same code. It is the part that deals with printing
the --more-- prompt and reading user input. I decided to move them
into a separate function.

And it would be very nice to have this completion context specific
depending on the command. E.g. for test it would display existing
tests, for ipc list of existing tasks etc. But this is just an idea.

Because the patch (except for the problem with 64bits which is easy to
fix) looks okay to me, I already applied it to the mainline (rev
1539). Thanks once more.

Regards,
- Vojta

[1] http://trac.helenos.org/wiki/Printf

>
>
> On Fri, Jun 29, 2012 at 12:38 PM, Vojtech Horky
<vojtech.horky@...> wrote:
>>
>> Hello Sandeep Kumar,
>> sorry for replying so late. I hope you are still with us .
>>
>> 2012/6/10 Sandeep Ruhil <sandeep.kumar.ruhil@...>:
>> > Hey,
>> >
>> > I am encountering a small problem in working on second point of this ticket.
>> >
>> > I need an object of indev_t inside the function symtab_compl of file
>> > kernel/generic/src/debug/symtab.c but when i include the header file
>> > <console/chardev.h> (containing the definition of indev_t) in the file
>> > kernel/generic/include/symtab.h (containing the prototype of symtab_compl
>> > function), i get the following compilation error :
>> >
>> > In file included from generic/include/console/chardev.h:40:0,
>> >          from generic/include/symtab.h:39,
>> >          from generic/include/debug.h:39,
>> >          from generic/include/synch/spinlock.h:42,
>> >          from generic/include/proc/scheduler.h:38,
>> >          from arch/ia32/src/proc/scheduler.c:35:
>> > generic/include/synch/waitq.h:56:2: error: expected specifier-qualifier-list
>> > before 'IRQ_SPINLOCK_DECLARE'
>> >
>> > When i inspected the reason, it turned out that :
>> >     spinlock.h includes debug.h
>> >     debug.h includes symtab.h
>> >     symtab.h includes chardev.h (my include)
>> >     chardev.h includes waitq.h
>> >     waitq.h uses IRQ_SPINLOCK_DECLARE, which is declared in spinlock.h but
>> > spinlock.h is not fully included as the compiler is following an inclusion
>> > of spinlock.h file and hence compiler cannot find the definition of
>> > IRQ_SPINLOCK_DECLARE macro.
>> >
>> > What can i do to resolve this error?
>> I would go for splitting the symtab.h into two files. Keep the lookup
>> functionality in one and move printing functions (symtab_print_search
>> and symtab_compl) to another one.
>>
>> This would not break any files except those under console/*.c so it
>> shall be pretty painless. The only problem is that these functions
>> require the symtab_search_one() function that is private (i.e. static)
>> to symtab.c. But I think that it is okay to make this function public
>> and move it to symtab.h.
>>
>> Hope this helps.
>>
>> Regards,
>> - Vojta
>>
>>
>> >
>> >
>> > --
>> > Regards,
>> > Sandeep Kumar
>> >
>> >
>> > _______________________________________________
>> > HelenOS-devel mailing list
>> > HelenOS-devel@...
>> > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
>> >
>>
>> _______________________________________________
>> HelenOS-devel mailing list
>> HelenOS-devel@...
>> http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
>
>
>
>
> --
> Regards,
> Sandeep Kumar
>
> _______________________________________________
> HelenOS-devel mailing list
> HelenOS-devel@...
> http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
>