Skip to content

MINIFICPP-1325 - Separate process group processor lookups for IDs and names, allow constructing ID-s from strings#865

Closed
hunyadi-dev wants to merge 3 commits intoapache:mainfrom
hunyadi-dev:MINIFICPP-1325_UUIDs_and_names
Closed

MINIFICPP-1325 - Separate process group processor lookups for IDs and names, allow constructing ID-s from strings#865
hunyadi-dev wants to merge 3 commits intoapache:mainfrom
hunyadi-dev:MINIFICPP-1325_UUIDs_and_names

Conversation

@hunyadi-dev
Copy link
Contributor

This is a cleanup PR before I start a refactor on configuration parsing, IDs and names lookups.

… names, allow constructing ID-s from strings
@hunyadi-dev hunyadi-dev force-pushed the MINIFICPP-1325_UUIDs_and_names branch from 884939e to a959ef6 Compare August 7, 2020 12:03
Copy link
Member

@szaszm szaszm left a comment

Choose a reason for hiding this comment

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

For some reason I couldn't reply to the other review comments, so writing it here:
+1 for explicitly listing the captured variables, just for clarity, not safety.

@hunyadi-dev
Copy link
Contributor Author

hunyadi-dev commented Aug 12, 2020

For some reason I couldn't reply to the other review comments, so writing it here:
+1 for explicitly listing the captured variables, just for clarity, not safety.

I would say listing the captured variables is just clutter-code in these cases. The lambdas mentioned capture everything in scope: there are no locals, this is captured and the single function args are also inputs for the lambda.

namespace minifi {
namespace utils {

const char* Identifier::UUID_FORMAT_STRING = "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx";
Copy link
Contributor

Choose a reason for hiding this comment

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

would constexpr allow us to make it an in-class initializer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it only works in c++17 or maybe even c++14.

Copy link
Contributor

Choose a reason for hiding this comment

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

you are right that from c++17 inline comes to the rescue, but even in c++11 we may (must) add an initializer to the declaration of a literal constexpr variable (and it is enough if it is not odr-used) (see here in section Constant static members)

#include <stdio.h>

struct A{
  static constexpr const char* FORMAT = "%d\n";
};

int main(){
  // uncommenting the following will generate a linker error
  // auto& ref = A::FORMAT;
  printf(A::FORMAT, 5);
}

it is not pivotal to move it to the header, I just wanted to share that it is possible

Copy link
Member

Choose a reason for hiding this comment

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

Please make sure I can't reassign the string to something else. constexpr or top-level const achieves that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made UUID_FORMAT_STRING constexpr.

@arpadboda arpadboda closed this in e1d8fe8 Sep 22, 2020
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.

5 participants