Message ID | 20190122011436.18408-1-blp@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev] seq: Correct example in comment. | expand |
On 22.01.2019 4:14, Ben Pfaff wrote: > It was deceptive for the example to imply that a seq can be declared > directly, because the API only allows for creating a new one on the heap. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > lib/seq.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > Thanks. Maybe you can additionally fix the C99-style comment here to not use a bad style inside the example. Anyway, Acked-by: Ilya Maximets <i.maximets@samsung.com> > diff --git a/lib/seq.h b/lib/seq.h > index 221ab9acddc5..92743c1eb4ea 100644 > --- a/lib/seq.h > +++ b/lib/seq.h > @@ -77,14 +77,14 @@ > * > * struct ovs_mutex mutex; > * struct ovs_list queue OVS_GUARDED_BY(mutex); > - * struct seq nonempty_seq; > + * struct seq *nonempty_seq; > * > * To add an element to the queue: > * > * ovs_mutex_lock(&mutex); > * ovs_list_push_back(&queue, ...element...); > * if (ovs_list_is_singleton(&queue)) { // The 'if' test here is optional. > - * seq_change(&nonempty_seq); > + * seq_change(nonempty_seq); > * } > * ovs_mutex_unlock(&mutex); > * > @@ -92,7 +92,7 @@ > * > * ovs_mutex_lock(&mutex); > * if (ovs_list_is_empty(&queue)) { > - * seq_wait(&nonempty_seq, seq_read(&nonempty_seq)); > + * seq_wait(nonempty_seq, seq_read(nonempty_seq)); > * } else { > * poll_immediate_wake(); > * } >
On Tue, Jan 22, 2019 at 12:52:56PM +0300, Ilya Maximets wrote: > On 22.01.2019 4:14, Ben Pfaff wrote: > > It was deceptive for the example to imply that a seq can be declared > > directly, because the API only allows for creating a new one on the heap. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > Thanks. > Maybe you can additionally fix the C99-style comment here to not use > a bad style inside the example. > > Anyway, > Acked-by: Ilya Maximets <i.maximets@samsung.com> Thanks, I applied this to master. It's tricky to avoid C99 comments in code samples inside regular C comments, because C comments don't nest. Do you have a suggestion on how to do it?
On 23.01.2019 1:43, Ben Pfaff wrote: > On Tue, Jan 22, 2019 at 12:52:56PM +0300, Ilya Maximets wrote: >> On 22.01.2019 4:14, Ben Pfaff wrote: >>> It was deceptive for the example to imply that a seq can be declared >>> directly, because the API only allows for creating a new one on the heap. >>> >>> Signed-off-by: Ben Pfaff <blp@ovn.org> >> >> Thanks. >> Maybe you can additionally fix the C99-style comment here to not use >> a bad style inside the example. >> >> Anyway, >> Acked-by: Ilya Maximets <i.maximets@samsung.com> > > Thanks, I applied this to master. > > It's tricky to avoid C99 comments in code samples inside regular C > comments, because C comments don't nest. Do you have a suggestion on > how to do it? Oops. Yes, you're right, it's a bit tricky. So, I'm not sure if this needed, but I see 2 possible solutions: 1. Add some spaces like / * Comment * /. But this increases the line length over 79 chars here. 2. Make it look not like a comment at all. i.e. Make it look like a a simple text. For example: if (ovs_list_is_singleton(&queue)) { <-- The 'if' test here is optional. This doesn't give a bad comment example and obviously should be removed while copying to the real code. Best regards, Ilya Maximets.
On Wed, Jan 23, 2019 at 11:34:22AM +0300, Ilya Maximets wrote: > On 23.01.2019 1:43, Ben Pfaff wrote: > > On Tue, Jan 22, 2019 at 12:52:56PM +0300, Ilya Maximets wrote: > >> On 22.01.2019 4:14, Ben Pfaff wrote: > >>> It was deceptive for the example to imply that a seq can be declared > >>> directly, because the API only allows for creating a new one on the heap. > >>> > >>> Signed-off-by: Ben Pfaff <blp@ovn.org> > >> > >> Thanks. > >> Maybe you can additionally fix the C99-style comment here to not use > >> a bad style inside the example. > >> > >> Anyway, > >> Acked-by: Ilya Maximets <i.maximets@samsung.com> > > > > Thanks, I applied this to master. > > > > It's tricky to avoid C99 comments in code samples inside regular C > > comments, because C comments don't nest. Do you have a suggestion on > > how to do it? > > Oops. Yes, you're right, it's a bit tricky. > > So, I'm not sure if this needed, but I see 2 possible solutions: > > 1. Add some spaces like / * Comment * /. > But this increases the line length over 79 chars here. > > 2. Make it look not like a comment at all. i.e. Make it look like a > a simple text. For example: > > if (ovs_list_is_singleton(&queue)) { <-- The 'if' test here is optional. > > This doesn't give a bad comment example and obviously should be removed > while copying to the real code. > > Best regards, Ilya Maximets. Good ideas. I like #2 better than #1. I sent a fix.
diff --git a/lib/seq.h b/lib/seq.h index 221ab9acddc5..92743c1eb4ea 100644 --- a/lib/seq.h +++ b/lib/seq.h @@ -77,14 +77,14 @@ * * struct ovs_mutex mutex; * struct ovs_list queue OVS_GUARDED_BY(mutex); - * struct seq nonempty_seq; + * struct seq *nonempty_seq; * * To add an element to the queue: * * ovs_mutex_lock(&mutex); * ovs_list_push_back(&queue, ...element...); * if (ovs_list_is_singleton(&queue)) { // The 'if' test here is optional. - * seq_change(&nonempty_seq); + * seq_change(nonempty_seq); * } * ovs_mutex_unlock(&mutex); * @@ -92,7 +92,7 @@ * * ovs_mutex_lock(&mutex); * if (ovs_list_is_empty(&queue)) { - * seq_wait(&nonempty_seq, seq_read(&nonempty_seq)); + * seq_wait(nonempty_seq, seq_read(nonempty_seq)); * } else { * poll_immediate_wake(); * }
It was deceptive for the example to imply that a seq can be declared directly, because the API only allows for creating a new one on the heap. Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/seq.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)