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

Bug #1289 #1441

Merged
merged 14 commits into from
Oct 29, 2015
Merged

Bug #1289 #1441

merged 14 commits into from
Oct 29, 2015

Conversation

fortizc
Copy link
Contributor

@fortizc fortizc commented Oct 28, 2015

Fixes #1289

@@ -34,6 +34,12 @@
#include "serviceRoutines/postUpdateContext.h"


static bool legalEntityLength(Entity* eP, const std::string &servicePath)
{
const int constant = 10;
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to use a #define in the file preamble (i.e. just after the #include lines):

#define STRUCTURAL_OVERHEAD_BSON_ID 10

(The name is to align with the terms used by MongoDB documentation... "contant" is too broad)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But for modern C and C++ the recommendation is use constants instead of #define maybe create a global file scoped constant? something like:
statict const int STRUCTURAL_OVERHEAD_BSON_ID = 10;

Copy link
Member

Choose a reason for hiding this comment

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

#define (the one that we typically use in CB) has the advantage of pre-processor text replacement (i.e. faster code... although maybe marginally faster) while static const has the advantage of allowing semantics in the constant (so it is treated as int in every place were it is used).

Both are ok, not an important issue at this point (use the one you prefer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 618422ee731b3896bd061782e740afb6dbb76de3

@fgalan
Copy link
Member

fgalan commented Oct 29, 2015

LGTM

@kzangeli
Copy link
Member

LGTM, but must be merged with develop before we can merge

fgalan pushed a commit that referenced this pull request Oct 29, 2015
@fgalan fgalan merged commit 5b02a10 into telefonicaid:develop Oct 29, 2015
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.

3 participants