Project

General

Profile

Bug #98

reserved identifier violation

Added by Markus Elfring almost 14 years ago. Updated over 13 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
% Done:

0%

Source:
Tags:
Backport:
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Pull request ID:
Crash signature (v1):
Crash signature (v2):

Description

This issue corresponds to my previous "feature request".

I suggest to try the search pattern "\<_(?:(?:_(.*))|([A-Z]+))" on source files. A couple of places will be found where names begin with two underscores or an underscore and an uppercase letter.

Examples:

This does not fit to the expected naming conventions of the C language standard.

See also

98.tbz - update suggestion (25.7 KB) Markus Elfring, 06/12/2010 12:10 PM

History

#1 Updated by Sage Weil almost 14 years ago

Yeah, I think all the headers #ifdef guards can be changed from __FOO_H to CEPH_FOO_H or similar. Patches welcome!

The _WorkQueue can also be renamed easily enough.

#2 Updated by Markus Elfring almost 14 years ago

Leading underscores can be avoided for various identifiers.

How do you think about the application of a name pattern like for the issue "#2852162: Unique names for include guards"?

#3 Updated by Sage Weil almost 14 years ago

No need for the trailing gibberish, I think.. the trailing H and preceding CEPH should be sufficient.

#4 Updated by Markus Elfring almost 14 years ago

I suggest to append a kind of UUID as a suffix to make include guards really unique.

Do any identifiers with leading underscores belong to a published application programming interface where do you need to keep them for backward compatibility of your source files?

#5 Updated by Sage Weil almost 14 years ago

The header ifdef guards should definitely be fixed. I think

#ifndef CEPH_FOO_H
#define CEPH_FOO_H

should be sufficient (i.e. CEPH_ prefix and H suffix). The only conflicts would be someone else using CEPH (their problem) and us using _H (which we don't).

#6 Updated by Sage Weil almost 14 years ago

Markus Elfring wrote:

I suggest to append a kind of UUID as a suffix to make include guards really unique.

Do any identifiers with leading underscores belong to a published application programming interface where do you need to keep them for backward compatibility of your source files?

Nope, and even if there are, it's early enough that I'm comfortable breaking them. Feel free to change them.

#7 Updated by Sage Weil almost 14 years ago

The qemu guys were worried about this when we submitted the rbd driver. Simply changing __FOO_H to CEPH_FOO_H throughout would be a nice cleanup.

#8 Updated by Markus Elfring almost 14 years ago

Would you like to integrate any changes from the appended name adjustments into your source code repository?

#9 Updated by Sage Weil almost 14 years ago

  • Status changed from New to Resolved

Okay, applied these with a few fixes (and without the trailing gibberish in #1). Thanks Markus!

#10 Updated by Markus Elfring almost 14 years ago

Would you also like to adjust any symbols with the prefix "_Included"?
Example: source:src/client/hadoop/org_apache_hadoop_fs_ceph_CephTalker.h@c1cede1023360bb2b023f362f2fa5ec73f2cb257#L5

Should this open issue be forwarded to the corresponding software project?

#11 Updated by Sage Weil almost 14 years ago

Markus Elfring wrote:

Would you also like to adjust any symbols with the prefix "_Included"?
Example: source:src/client/hadoop/org_apache_hadoop_fs_ceph_CephTalker.h@c1cede1023360bb2b023f362f2fa5ec73f2cb257#L5

Should this open issue be forwarded to the corresponding software project?

Sure. It should be fixed in ceph.git first. This hasn't been merged into hadoop.

#12 Updated by Markus Elfring almost 14 years ago

Who is responsible for the tool that generates the affected header files?

#13 Updated by Sage Weil almost 14 years ago

Markus Elfring wrote:

Who is responsible for the tool that generates the affected header files?

Oh.. if those are generated headers, it's probably JNI. I wouldn't worry about it in that case (at least not until is actually causes a problem for someone).

#14 Updated by Markus Elfring almost 14 years ago

Do you mean that a tool for the Java™ Native Interface is affected?

#15 Updated by Sage Weil almost 14 years ago

Markus Elfring wrote:

Do you mean that a tool for the Java™ Native Interface is affected?

They are generated by some java tool, yeah.

/* DO NOT EDIT THIS FILE - it is machine generated */
#include <jni.h>
/* Header for class org_apache_hadoop_fs_ceph_CephTalker */

#ifndef _Included_org_apache_hadoop_fs_ceph_CephTalker
#define _Included_org_apache_hadoop_fs_ceph_CephTalker
#ifdef __cplusplus
...

#16 Updated by Markus Elfring almost 14 years ago

Do you use the tool "javah" for these header files?

#17 Updated by Greg Farnum almost 14 years ago

I don't actually remember how they were created, but according to the Readme file, it's javah, yes.
Those files aren't in the Hadoop repository, though; they're only used in creating the C++ CephFSInterface library which we will manage even if they do accept the userspace client.

Also available in: Atom PDF