GetModuleOrder() result has unspecified order of destruction

[dekimir]

Currently, GetModuleOrder() returns a reference to a static vector object. This is a violation of the style guide because other static objects may accidentally dereference this in their destructors, without any guarantee that the GetModuleOrder vector isn't already destroyed. Instead of creating a static vector, the method should either use a C array or a static pointer to a dynamically allocated vector.

References:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2

[umar456]

There is a provision for this in the Google style guide.

(This prohibition does not apply to a static variable within function scope, since its initialization order is well-defined and does not occur until control passes through its declaration.)

Also since static objects are not allowed by the style guide, this should not be an issue. The only objects which CAN call this function during destruction would be function local objects with static lifetimes. The destruction of these objects is well defined by the standard. If one of the function local objects were to access this method in the destructor then you can define the order of creation(and therefore destruction) by calling the GetModuleOrder before the other object is created.

int main() {
GetModuleOrder();
OtherFunctionWhichDependsOnGetModuleOrderInDestructor();
...
// Profit??
}

This approach is also thread safe in the C++11 standard. Reference: http://stackoverflow.com/a/8102145/523374
Where as the dynamically allocated vector would require synchronization mechanism to achieve the same effect.

[dekimir]

Umar, I'm not sure I agree with your guide interpretation. The exception you quote does not reverse the ban on function static variables (additionally reinforced by the second paragraph). Instead, it's an exception to the sentence immediately preceding it:

Therefore in addition to banning globals of class type, we do not allow static POD variables to be initialized with the result of a function...

Here's how the dangling-reference danger could play out:

struct Foo {
  Foo(const vector<vector<SpvOp>>& moduleOrder) : mo_(moduleOrder) {}
  ~Foo() { cout << mo_[0][0] << endl; }
  const vector<vector<SpvOp>>& mo_;
}

int main() {
  static Foo foo(GetModuleOrder());
}

Although mo_ is a reference to a static variable, that variable may be destroyed before foo's destructor is called.

[umar456]

The order in which Foo and the module order vector is well defined in the example you posted. The code you posed will yield the correct(intended?) results because the GetModuleOrder function is called before the Foo object is initialized. Here is an example where it would cause issues you brought up:

struct Foo {
  Foo() {}
  ~Foo() { cout << GetModuleOrder()[0][0] << endl; }
}

int main() {
  static Foo foo();
  GetModuleOrder();
  return 0;
}

This code will cause a problem because GetModuleOrder is called after the static Foo object is initialized. And since order of destruction is the opposite of the other of construction the vector module is called In order to fix this type of issue all you would need to do is to call GetModuleOrder before the Foo construction like so:

int main() {
  GetModuleOrder();
  static Foo foo();
  return 0;
}

Anyway, the only reason the Google style guide gives is that the order is not defined for static objects. Function local static objects have a defined order of construction and destruction so I don't think that rule should apply to those objects.

[dekimir]

Thanks for correcting my example; I agree that it didn't illustrate the danger well because of the deterministic destruction order. Of course, it's still too easy to accidentally reorder GetModuleOrder() and foo() -- two lines that don't appear related to each other when you look at them alone -- and get slapped with undefined behaviour.

I'm not comfortable with ignoring parts of the style guide (however much I may find them objectionable) without a broad agreement and a written-down list of exceptions first.

[dneto0]

The function is only used to support ValidationState_t::isOpcodeInCurrentLayoutStage
That's a table lookup that could be implemented as a set of switch statements. That would eliminate the complexity and I think would be at least as readable.