Skip to content
This repository was archived by the owner on Jun 10, 2025. It is now read-only.

Conversation

@sorc1
Copy link
Contributor

@sorc1 sorc1 commented Sep 20, 2016

The patches introduce Pack()/Unpack() functions for Multi scanner to serialize/deserialize with msgpack.

Copy link
Contributor

@starius starius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comments:

  1. Style: add braces around one line blocks, please.
  2. Why not protocol buffer?


void Pack(yostream* s)
{
char buf[PACKED_HEADERS * 8 * 4];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the meaning of 4? Could you replace 8 and 4 with named constants, please?

static void PackScanner(const Scanner<Relocation, Shortcutting>& scanner, yostream* s)
{
Pire::Header hdr(1, 0);
char buf[256];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the meaning of 256? Could you replace it with named constants, please?

Pire::Header hdr(1, 0);
char buf[256];
char *ptr;
size_t i;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this declaration to the loop where it is used.

{
Pire::Header hdr(1, 0);
char buf[256];
char *ptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare a variable at the same line with setting its initial value, please.

typedef Scanner<Relocation, Shortcutting> ScannerType;
Pire::Header hdr(1, 0);
Scanner<Relocation, Shortcutting> sc;
size_t i;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to the line where it is used first time.


template <class Header>
static void SetMaskRaw(Header& header, size_t ind, size_t val)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/return//


template <class Header>
static void SetMask(Header& header, size_t ind, char c)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use SetMaskRaw here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain what did you mean? What SetMaskRaw should I use here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetMaskRaw<Header>(header, ind, FillSizeT(c));

#endif
#if defined(__cplusplus) && !defined(__STDC_LIMIT_MACROS)
#define __STDC_LIMIT_MACROS 1 /* make С++ to be happy */
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain this code more carefully, please.

@sorc1
Copy link
Contributor Author

sorc1 commented Sep 21, 2016

21.09.2016 03:39, Boris Nagaev wrote:

@starius requested changes on this pull request.

General comments:

  1. Style: add braces around one line blocks, please.
  2. Why not protocol buffer?

Some reasons for that:

  1. No extra build dependencies
  2. The format is not going to change often
  3. It's just simple: while packing/unpacking, we're working
    directly with the scanner instance's memory, not with
    objects-to-serialize

I'll fix the problems you've pointed on and then I'll resend the patches.

@sorc1 sorc1 closed this Sep 21, 2016
@sorc1
Copy link
Contributor Author

sorc1 commented Sep 21, 2016

#46

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants