diff mbox series

[ovs-dev] seq: Correct example in comment.

Message ID 20190122011436.18408-1-blp@ovn.org
State Accepted
Headers show
Series [ovs-dev] seq: Correct example in comment. | expand

Commit Message

Ben Pfaff Jan. 22, 2019, 1:14 a.m. UTC
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(-)

Comments

Ilya Maximets Jan. 22, 2019, 9:52 a.m. UTC | #1
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();
>   *    }
>
Ben Pfaff Jan. 22, 2019, 10:43 p.m. UTC | #2
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?
Ilya Maximets Jan. 23, 2019, 8:34 a.m. UTC | #3
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.
Ben Pfaff Jan. 23, 2019, 8:09 p.m. UTC | #4
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 mbox series

Patch

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();
  *    }