Skip to content

Conversation

@cringti
Copy link
Contributor

@cringti cringti commented Apr 15, 2019

Removes whitespace from the C common header template. This whitespace
separates the template's doxygen-style comment and the user's
(optional) trailing interface comment.

This helps remove undesirable trailing whitespace for the (many) users
that don't use trailing interface comments.

Signed-off-by: Chris Ring cring@ti.com

Removes whitespace from the C common header template.  This whitespace
separates the template's doxygen-style comment and the user's
(optional) trailing interface comment.

This helps remove undesirable trailing whitespace for the (many) users
that don't use trailing interface comments.

Signed-off-by: Chris Ring <cring@ti.com>
{$fn.prototype};{$fn.ilComment}{$loop.addNewLineIfNotLast}
{% endfor -- functions %}
//@} {$iface.ilComment}
//@}{$iface.ilComment}
Copy link
Member

@Hadatko Hadatko Jan 2, 2020

Choose a reason for hiding this comment

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

Hi, could you change this line to use space in case ilcomment is not empty? like:

//@}{% if iface.ilComment %} {$iface.ilComment}{% endif %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally got a chance to test this, and I'd like to make sure we're on the same page.

Consider the following .erpc snippet from a modified examples/temp_alarm.erpc file - note the single space between the } and / on the last line.

//! Asynchronous events from M4 to Linux.
interface TempAsync
{
    //! Void return so we can verify the message was received.
    alarm_fired(SensorAddress addr, float temp) -> void

    //! Oneway since it's less important than an alarm.
    oneway read_results(list<SensorReadResult> results @length(count), uint32 count)
} /**< test comment */

With my existing PR, this is what gets generated (note the nice "you get the same whitespace as in your eprc file" single space on the last line):

//! Asynchronous events from M4 to Linux.
//! @name TempAsync
//@{
//! Void return so we can verify the message was received.
void alarm_fired(SensorAddress addr, float temp);

//! Oneway since it's less important than an alarm.
void read_results(const SensorReadResult * results, uint32_t count);
//@} /**< test comment */

With the change you propose, you'd instead get this (note the less-nice-IMHO "you get an extra space before your erpc file's comment" double space on the last line):

//! Asynchronous events from M4 to Linux.
//! @name TempAsync
//@{
//! Void return so we can verify the message was received.
void alarm_fired(SensorAddress addr, float temp);

//! Oneway since it's less important than an alarm.
void read_results(const SensorReadResult * results, uint32_t count);
//@}  /**< test comment */

I'd prefer my current commit where you get exactly the comment line you have in your erpc file (rather than unconditionally getting a leading space added), but I'll change this to whichever you like. This commit is just about removing trailing whitespace when there is no trailing comment.

Let me know and I can rebase "whatever you want" onto the develop branch so you can merge.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @cringti, i thought you are pointing different issue which would be fixed way i proposed. But this looks like a bug where all spaces should be removed via parser and then should be added one this way. I will do test on my side. And i will let you know.

Copy link
Member

Choose a reason for hiding this comment

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

I am appologizing. Your correction looks right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No issue, thanks for checking. Let me know if you need anything else from me. Unless I hear back, I'm considering my side of things done.

@MichalPrincNXP
Copy link
Member

Hello @cringti , could you please rebase your PR to the develop branch? Thanks.

@Hadatko
Copy link
Member

Hadatko commented Jan 27, 2020

@MichalPrincNXP The request looks right from my side. Can be merged.

@MichalPrincNXP MichalPrincNXP merged commit bd8bf57 into EmbeddedRPC:develop Jan 28, 2020
@MichalPrincNXP
Copy link
Member

Thank you @cringti for your pull request.

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

Development

Successfully merging this pull request may close these issues.

3 participants