Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tutorial 1 and 2 #399

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

tutorial 1 and 2 #399

wants to merge 1 commit into from

Conversation

apre
Copy link

@apre apre commented Jan 12, 2017

Hi,
Here are the tutorial 1 - and 2 example files, with now a compatible license.

kind regards,

Adrien


This change is Reviewable

also remove some build warnings.
@alanxz
Copy link
Owner

alanxz commented Feb 7, 2017

Review status: 0 of 5 files reviewed at latest revision, 15 unresolved discussions.


a discussion (no related file):
Apologies for taking so long to get to this, this is really a fantastic start.

First round of comments here.


examples/new_task.c, line 17 at r1 (raw file):

  • Portions created by Alan Antonuk are Copyright (c) 2012-2013
  • Alan Antonuk. All Rights Reserved.
  • Portions created by Mike Steinert are Copyright (c) 2012-2013
  • Mike Steinert. All Rights Reserved.
  • Portions created by VMware are Copyright (c) 2007-2012 VMware, Inc.
  • All Rights Reserved.
  • Portions created by Tony Garnock-Jones are Copyright (c) 2009-2010
  • VMware, Inc. and Tony Garnock-Jones. All Rights Reserved.

You can remove all of this, just leave your contribution (here and throughout).


examples/new_task.c, line 55 at r1 (raw file):

#include <amqp_tcp_socket.h>
#include <amqp.h>
#include <amqp_framing.h>

amqp.h includes this. No need to include this here.


examples/new_task.c, line 63 at r1 (raw file):

    char const *hostname= "localhost";
    int port= 5672;
    int  status;

Please format all of the source-code files with clang-format (https://clang.llvm.org/docs/ClangFormat.html). The .clang-format file in the root of the repository should set the correct coding standard.


examples/new_task.c, line 74 at r1 (raw file):

    amqp_rpc_reply_t r;

    (void) argc; (void ) argv; // get rid of the unused variable warning

Just drop both of these from the function signature (here and throughout).


examples/new_task.c, line 86 at r1 (raw file):

    r=amqp_login(conn, "/", 0, 131072, 0, AMQP_SASL_METHOD_PLAIN, "guest", "guest");
    assert(r.reply_type==AMQP_RESPONSE_NORMAL);

Ideally we shouldn't use assert().

Use an if-statement like whats done with amqp_socket_open above.


examples/new_task.c, line 88 at r1 (raw file):

    assert(r.reply_type==AMQP_RESPONSE_NORMAL);

    amqp_channel_open(conn, 1);

The correct path for error-checking for RPC-type calls is to check for NULL, which indicates an error. If the result is NULL, then use amqp_get_rpc_reply() to get the details of what failed (the existing examples are bad at this).

amqp_channel_open_ok_t* res = amqp_channel_open(conn, 1);
if (res == NULL) {
  /* Use amqp_get_rpc_reply() to get details on what failed */
  printf("failed to open channel\n");
  exit(1);
...

examples/new_task.c, line 88 at r1 (raw file):

    assert(r.reply_type==AMQP_RESPONSE_NORMAL);

    amqp_channel_open(conn, 1);

Use a variable for the channel number, and pass it into to other functions that require a channel.


examples/new_task.c, line 101 at r1 (raw file):

                                                         1,
                                                         amqp_cstring_bytes(queueName),
                                                         //  amqp_empty_bytes,

Whats commented out here?


examples/new_task.c, line 102 at r1 (raw file):

                                                         amqp_cstring_bytes(queueName),
                                                         //  amqp_empty_bytes,
                                                         0,

Add comment indicating what this means


examples/new_task.c, line 103 at r1 (raw file):

                                                         //  amqp_empty_bytes,
                                                         0,
                                                         1,// durable

Use the old-school /* */ comments (here and throughout).


examples/new_task.c, line 104 at r1 (raw file):

                                                         0,
                                                         1,// durable
                                                         0,0,

Add comments indicating what each of these mean (here and throughout - (this is an unfortunate failing of the library)).


examples/new_task.c, line 107 at r1 (raw file):

                                                         amqp_empty_table);

        r = amqp_get_rpc_reply(conn);

Same deal as with amqp_channel_open() above. amqp_queue_declare has failed if rq is NULL, use amqp_get_rpc_reply to get details about the error under this condition.


examples/new_task.c, line 133 at r1 (raw file):

    }

    r=amqp_channel_close(conn, 1, AMQP_REPLY_SUCCESS);

No need to do this, channels are implicitly closed when the connection is closed.


examples/new_task.c, line 135 at r1 (raw file):

    r=amqp_channel_close(conn, 1, AMQP_REPLY_SUCCESS);

    r=amqp_connection_close(conn, AMQP_REPLY_SUCCESS); assert(r.reply_type==AMQP_RESPONSE_NORMAL);

Use a non-assert() error checking


Comments from Reviewable

@apre
Copy link
Author

apre commented Feb 12, 2017

Hi,

Thank you very much for your review!
I will rework that this month.

@apre
Copy link
Author

apre commented Feb 14, 2017

Review status: 0 of 5 files reviewed at latest revision, 15 unresolved discussions.


examples/new_task.c, line 55 at r1 (raw file):

Previously, alanxz (Alan Antonuk) wrote…

amqp.h includes this. No need to include this here.

Apparently, ’amqp_tcp_socket.h’ isn't included in ’amqp.h’, so I still need to keep that include, otherwise I got build warning.


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants