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

Modified name for common types macro scope. #337

Conversation

Hadatko
Copy link
Member

@Hadatko Hadatko commented Jan 10, 2023

Signed-off-by: Cervenka Dusan cervenka@acrios.com

Pull request

Choose Correct

  • bug
  • feature

Describe the pull request

Description #336
Default change from ERPC_TYPE_DEFINITIONS to ERPC_TYPE_DEFINITIONS_erpcShim
Default with program name change from ERPC_TYPE_DEFINITIONS to ERPC_TYPE_DEFINITIONS_
Using annotation value for @scope_name change from ERPC_TYPE_DEFINITIONS to ERPC_TYPE_DEFINITIONS_

To Reproduce

See tests

Expected behavior

See tests

Screenshots

Desktop (please complete the following information):

  • OS:
  • eRPC Version:

Steps you didn't forgot to do

  • I checked if other PR isn't solving this issue.
  • I read Contribution details and did appropriate actions.
  • PR code is tested.
  • PR code is formatted.
  • Allow edits from maintainers pull request option is set (recommended).

Additional context

@Hadatko Hadatko added this to the 1.11.0 milestone Jan 10, 2023
@Hadatko Hadatko self-assigned this Jan 10, 2023
@Hadatko Hadatko linked an issue Jan 10, 2023 that may be closed by this pull request
@Diagano
Copy link

Diagano commented Jan 10, 2023

Not that its mattrers much but maybe convert to uppercase in the template use of the scope_name?
{$upper(scope_name)}

@Hadatko
Copy link
Member Author

Hadatko commented Jan 10, 2023

Yeah i was thinking about that and i wanted to let it on user. User can use upper case in his definition.
@MichalPrincNXP what do you think?

@Diagano
Copy link

Diagano commented Jan 10, 2023

Well two points about that:

  1. User choice in that case will also impact file naming
  2. Use of lower case as default also counters that in some sense.

@Hadatko
Copy link
Member Author

Hadatko commented Jan 10, 2023

Well make sense. But i need distuinguish between annotation value and program name and use upper case only when annotation was not used.

@Diagano
Copy link

Diagano commented Jan 10, 2023

Got it. As I mentioned, I lack the system wide view and impacts. Thank you!

@Hadatko
Copy link
Member Author

Hadatko commented Jan 12, 2023

@Diagano i decided to do it as you proposed. UpperCase. It is because the macro name is still with eRPC prefix and users shouldn't use it and be aware of its existention. Will see future requests :D

@Hadatko Hadatko force-pushed the feature/ERPC_TYPE_DEFINITION branch from 768a13c to 8587ba5 Compare January 12, 2023 09:40
@Hadatko
Copy link
Member Author

Hadatko commented Feb 13, 2023

@MichalPrincNXP what do you think about this PR?

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@MichalPrincNXP
Copy link
Member

MichalPrincNXP commented Mar 7, 2023

@MichalPrincNXP what do you think about this PR?

I agree with the approach. There are some build fails after the rebase however:

erpcgen/src/Generator.cpp:98:14: error: 'transform' is not a member of 'std'
   std::transform(scopeNameC.begin(), scopeNameC.end(), scopeNameC.begin(), ::toupper);

Probably, the #include "algorithm" will be needed.

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@MichalPrincNXP MichalPrincNXP merged commit 63181f3 into EmbeddedRPC:develop Mar 7, 2023
@MichalPrincNXP
Copy link
Member

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

ERPC_TYPE__DEFINITIONS when multiple distinct clients are used
3 participants