SterlingPeet / cookiecutter-fprime-deployment

Cookiecutter template for creating new deployments within an F Prime project/repository
Other
4 stars 0 forks source link

Broken support for FW_OBJECT_NAMES == 0 #5

Closed nathancheek closed 4 years ago

nathancheek commented 4 years ago

When FW_OBJECT_NAMES is not set to 1, the component class constructor gets broken.

See below, the constructor header is only defined inside the #if FW_OBJECT_NAMES == 1 block.

https://github.com/SterlingPeet/cookiecutter-fprime-deployment/blob/572ebe684829dec6481c3fefcca8e49f143fa5c8/%7B%7Bcookiecutter.deployment_dir_name%7D%7D/%7B%7Bcookiecutter.component_dir_name%7D%7D/%7B%7Bcookiecutter.component_name%7D%7DComponent%7B%7Bcookiecutter.component_explicit_common%7D%7D%7B%7Bcookiecutter.component_suffix%7D%7D.cpp#L18-L25

Compare to the constructor header declaration in the .hpp file. This should fix it:

#else
{{cookiecutter.component_name}}Component{{cookiecutter.component_explicit_common}}{{cookiecutter.component_suffix}}(void) :
{{cookiecutter.component_name}}Component{{cookiecutter.component_explicit_common}}Base(),
#endif
SterlingPeet commented 4 years ago

Well, thats a bug. The proposed fix is also buggy, but I agree that it should follow the pattern in the .cpp file. The cheetah template from F Prime upstream looks like this:


  // ----------------------------------------------------------------------
  // Construction, initialization, and destruction
  // ----------------------------------------------------------------------

  ${name}ComponentImpl ::
\#if FW_OBJECT_NAMES == 1
    ${name}ComponentImpl(
$emit_non_port_params([ $param_compName ])
    ) :
      ${component_base}(compName)
\#else
    ${name}ComponentImpl(void)
\#endif
  {

  }

So I think I will use that pattern.

SterlingPeet commented 4 years ago

I think the fix actually ends up being this:

  {{cookiecutter.component_slug}}{{cookiecutter.component_explicit_component_suffix}}{{cookiecutter.component_impl_suffix}} ::
#if FW_OBJECT_NAMES == 1
    {{cookiecutter.component_slug}}{{cookiecutter.component_explicit_component_suffix}}{{cookiecutter.component_impl_suffix}}(
        const char *const compName
    ) :
      {{cookiecutter.component_slug}}ComponentBase(compName),
#else
      {{cookiecutter.component_slug}}{{cookiecutter.component_explicit_component_suffix}}{{cookiecutter.component_impl_suffix}}(void),
#endif
nathancheek commented 4 years ago

I think it should not have , at the end of the alternate constructor header definition. Or does that work?

SterlingPeet commented 4 years ago

I believe you need the comma for constructor initialization of member variables, which I am using. So, yes I think it works just fine.

nathancheek commented 4 years ago

I just checked and that gives:

error: expected unqualified-id before '{' token

, is not a valid character to come directly after the constructor header definition. You could put : but only if followed by a member variable constructor initialization. Otherwise, it gives the same error as above. , is only meant for separating member variables in the constructor initialization section.

SterlingPeet commented 4 years ago

Oh, I see, you are correct, the #if segment has ) : but I messed up the #else segment by putting ),.

SterlingPeet commented 4 years ago

The fix that is currently on the devel branch should correct that, I'll wait on other updates to push it to master.