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

In C, double underscores should not be used at the start of an identifier #343

Closed
seanm opened this issue Feb 16, 2021 · 9 comments
Closed

Comments

@seanm
Copy link
Contributor

seanm commented Feb 16, 2021

I notice there were some changes recently like:

cba993c

- static void H5_debug_mask(const char*);
+ static void H5__debug_mask(const char*);

Where some things got renamed to have double underscores.

However, in C, all names containing a double underscore are reserved:

https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL51-CPP.+Do+not+declare+or+define+a+reserved+identifier

These days clang can warn about that.

So really that change above is kinda backwards. :)

@qkoziol
Copy link
Contributor

qkoziol commented Feb 19, 2021

It's definitely "marching to our own drummer", at the least. :-)

The use of underscores after the "H5*" prefix for function names in the HDF5 source code follows these rules:

  • Public routines: No '_' after the "H5*" prefix.
  • "Library private"-scoped routines: One '_' after the "H5*" prefix.
  • "Package"-scoped and static routines: Two '_' after the "H5*" prefix.

I believe our use of two '_' in package and static scoped function names predates the standard committee taking them away from user code. :-) I blame our non-compliance on the lack of namespace support in C. ;-)

I don't have an awesome suggestion to move away from using two '_' in the package and static scopes. I've tossed around various solutions and they mostly look awkward. :-/

@seanm
Copy link
Contributor Author

seanm commented Feb 19, 2021

Thanks for explaining the naming scheme, I understand the point of the change now.

More importantly, I confused C and C++, and in fact double underscore is OK within the identifier in C. Here's a link for C, not C++:

https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

But I didn't create this ticket for nothing, there are in fact many -Wreserved-id-macro warnings from clang.

A great many could be fixed by changing the include guard spelling:

#ifndef _H5ACmodule_H
#define _H5ACmodule_H

That could like be done with a regex.

@qkoziol
Copy link
Contributor

qkoziol commented Feb 19, 2021

Ah, excellent, glad to know that it's still OK in C. whew ;-)

Yes - I noticed the header file inclusion guard issue also and agree with you about fixing those.

@seanm
Copy link
Contributor Author

seanm commented Feb 19, 2021

What new spelling? Just drop the leading underscore?

@qkoziol
Copy link
Contributor

qkoziol commented Feb 19, 2021

Yes, that looks fine to me and is what the SEI page offers for a suggested fix.

@seanm
Copy link
Contributor Author

seanm commented Feb 19, 2021

OK cool. Seems sometimes 1 leading underscore is used, other times 2. These two regex seem to catch them all:

  • _H5(.*)_H - 90 matches in .h files
  • __H5(.*)_H - 501 matches in .h files

#361

@seanm seanm changed the title In C, double underscores should not be used in names In C, double underscores should not be used at the start of an identifier Feb 19, 2021
@derobins
Copy link
Member

Fixed in #361

@seanm
Copy link
Contributor Author

seanm commented Jun 3, 2022

Please reopen.

I just tried building from develop and there are still -Wreserved-id-macro warnings from include guards, for example:

hdf5/src/H5FDdevelop.h:19:9: warning: macro name is a reserved identifier [-Wreserved-id-macro]
#define _H5FDdevelop_H
        ^

I'll make a patch. But this will keep happening unless you add -Wreserved-id-macro to one of your buildbots... can you do that?

@seanm
Copy link
Contributor Author

seanm commented Jun 14, 2022

Additional fixes here: #1812

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

No branches or pull requests

3 participants