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

[core-c] removed ecal_ prefix of public API header files #1939

Merged
merged 7 commits into from
Jan 30, 2025

Conversation

rex-schilasky
Copy link
Contributor

Description

Removed the ecal_ prefix of all C language header.

@rex-schilasky rex-schilasky added the cherry-pick-to-NONE Don't cherry-pick these changes label Jan 23, 2025
@rex-schilasky rex-schilasky added this to the eCAL 6 milestone Jan 23, 2025
Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

I think, if we're renaming them, we should think about what is consistent.
Personally, I would prefer a completely new include directory, like ecal_c and then ecal_c/publisher.h etc.
This naming would emphasize that the c binding is a separate entity altogether.

@rex-schilasky
Copy link
Contributor Author

I think, if we're renaming them, we should think about what is consistent. Personally, I would prefer a completely new include directory, like ecal_c and then ecal_c/publisher.h etc. This naming would emphasize that the c binding is a separate entity altogether.

Should I apply this as well?

@KerstinKeller
Copy link
Contributor

Let's hear what @FlorianReimold and @Peguen say...

@rex-schilasky
Copy link
Contributor Author

Made a second commit renaming the c language ecal folder to ecalc. Lets decide which way to go.

@FlorianReimold
Copy link
Member

I would prefer ecal_c (it separates ecal and c better and is more visible). Dropping the cimpl from the header files as well would be nice, too, so we have ecal_c/publisher.h

@KerstinKeller
Copy link
Contributor

We decided ecal_c as a folder name would be good, because it's easier to spot / read.

If we do this, we should also

  • remove "_cimpl"
  • match the .c files to the header file names (I know we didn't yet do it for C++, but this always drives me crazy, if .c and .h files are named differently
  • rename ecalc.h -> ecal.h, ecalc_export.h -> export.h and ecalc_... -> ...
  • also think about if we want the same folder structure as for C++ (pubsub / client subfolder).

These are my remarks 😄

@rex-schilasky
Copy link
Contributor Author

We decided ecal_c as a folder name would be good, because it's easier to spot / read.

If we do this, we should also

  • remove "_cimpl"
  • match the .c files to the header file names (I know we didn't yet do it for C++, but this always drives me crazy, if .c and .h files are named differently
  • rename ecalc.h -> ecal.h, ecalc_export.h -> export.h and ecalc_... -> ...
  • also think about if we want the same folder structure as for C++ (pubsub / client subfolder).

These are my remarks 😄

That makes fully sense!

* @brief eCAL core function c interface
**/

#ifndef ecal_core_cimpl_h_included
#define ecal_core_cimpl_h_included
#ifndef core_h_included
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep canonical names here (including e.g. the folder structure), to avoid name clashes with other libraries (e.g. type_h_included might be something different libraries might define...

Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, we could just do #pragma once where the compiler will do the right thing.
not sure how well defined this is for C compilers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added folder structure (next commit) ..

@@ -18,12 +18,12 @@
*/

/**
* @file ecalc_export.h
* @file export.h
Copy link
Contributor

Choose a reason for hiding this comment

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

in c++ we kept the folder structure here (or did we?), maybe it makes sense here, too. @file ecal_c/export

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next commit ..

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

The file structure looks really good, but please see my other two comments.

@KerstinKeller KerstinKeller mentioned this pull request Jan 30, 2025
5 tasks
@rex-schilasky rex-schilasky merged commit a996f07 into master Jan 30, 2025
21 of 22 checks passed
@rex-schilasky rex-schilasky deleted the core/ecalc-header-renaming branch January 30, 2025 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants