refactor: acl helpers and global tables

[kostasrim]

Resolves #3287

• remove helper.h and helper.cc and move them to AclFamily class
• remove global tables from acl_commands_def and move them to AclFamily

[romange]

I see you gave up on using global variables at all. It's ok but that's not what I meant. Sometimes global variables make sense to avoid passing them around especially if their contents stay constant during the run of the program.

I meant that we should not fill data into dynamically allocated global variables before the program starts.

[romange]

in other words you could keep them, but just initialize them during the start sequence of the service.

[kostasrim]

Sure, but google style of code is against using global variables with non trivial destructors. Here is an example from the docs you referenced on the issue

// bad: non-trivial destructor
const std::string kFoo = "foo";

// Bad for the same reason, even though kBar is a reference (the
// rule also applies to lifetime-extended temporary objects).
const std::string& kBar = StrCat("a", "b", "c");

void bar() {
  // Bad: non-trivial destructor.
  static std::map<int, int> kData = {{1, 0}, {2, 0}, {3, 0}};
}

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Objects with static storage duration are forbidden unless they are trivially destructible.

Furthermore, I could have used

Note that references are not objects, and thus they are not subject to the constraints on destructibility. The constraint on dynamic initialization still applies, though. In particular, a function-local static reference of the form static T& t = *new T; is allowed.

but I dot like this idiom even though it applies on our case.

Do I agree with these rule? I do not (because if you know when you first call bar() everything is fine). But since we are following the guidelines I thought it would be best to apply them.

[kostasrim]

p.s.2 if I had to pass them around in many places I would have used the idiom above but it was only a handful one that made me lean toward removing the globals all together

[romange]

ok, sounds good.

[romange]

are these methods require this? I spotted at least some that do not. Why not put them into the anonymous space of the cc file?

[romange]

basically anything that does not belong to the class should not be part of its declaration

[kostasrim]

+1

[kostasrim]

This one in particular needs it because it calls CategoryToIdx() which is a method now of AclFamilly. I removed the ones that are independent and moved them in an unnamed namspace:)

I also cleaned up some of the interfaces that required the registry as a parameter since now they are methods :)

[romange]

looks good but I am surprised about the diff size of this PR.
I thought it would be as simple as removing the initialization of variables in header file and moving it to a function in cc file.

[kostasrim]

looks good but I am surprised about the diff size of this PR. I thought it would be as simple as removing the initialization of variables in header file and moving it to a function in cc file.

Found an opportunity to get rid of helper.cc + apply the google guidelines 🤷‍♂️