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

reserved identifier violation #874

Closed
elfring opened this issue Sep 25, 2019 · 22 comments
Closed

reserved identifier violation #874

elfring opened this issue Sep 25, 2019 · 22 comments
Labels
question Further information is requested wontfix This will not be worked on

Comments

@elfring
Copy link

elfring commented Sep 25, 2019

I would like to point out that identifiers like “RCLCPP_ACTION__QOS_HPP_” and “RCLCPP__EXECUTOR_HPP_do not fit to the expected naming convention of the C++ language standard.
Would you like to adjust your selection for unique names?

@dirk-thomas dirk-thomas added the question Further information is requested label Sep 25, 2019
@dirk-thomas
Copy link
Member

The ROS 2 code base uses a trailing underscore but no leading underscore so it doesn't match either of the two cases described in the referenced doc. Can you point to a reference why identifiers with a trailing underscore are a violation?

@sloretz
Copy link
Contributor

sloretz commented Sep 25, 2019

Are you referring to the double underscore?

Each name that contains a double underscore __ or begins with an underscore followed by an uppercase letter is reserved to the implementation for any use.

The double underscore is being used to make the include guards unique: google/styleguide#78 . It might be a reserved name, but it seems very unlikely conflict since the guards are so long.

@elfring
Copy link
Author

elfring commented Sep 25, 2019

How do you think about to avoid that this software depends on undefined behaviour?

@dirk-thomas
Copy link
Member

How do you think about to avoid that this software depends on undefined behaviour?

Just to include the relevant snippet here:

17.4.3.1.2 Global names [lib.global.names]
Certain sets of names and function signatures are always reserved to the implementation:
Each name that contains a double underscore (__) or begins with an underscore followed by an uppercase letter (2.11) is reserved to the implementation for any use.

That is a good reason to consider alternatives. Do you have a specific proposal with what the current identifiers could be replaced?

@elfring
Copy link
Author

elfring commented Sep 25, 2019

@dirk-thomas
Copy link
Member

Would you like to reduce the usage of double underscores in affected identifiers?

Wouldn't that result in the problem described in google/styleguide#78 which is the reason why we use double underscores as the directory separator?

How do you think about to use a kind of UUIDs for include guards?

Using #pragma once might be an option. We would need to make sure that it is available on all currently targeted platforms and check if it would be a problem for platforms we might want to target in the future.

@elfring
Copy link
Author

elfring commented Sep 25, 2019

I would prefer C++ standard compliant solutions.

@wjwwood
Copy link
Member

wjwwood commented Oct 8, 2019

I would prefer C++ standard compliant solutions.

To be clear (I think that's what you're implying) that means no #pragma once: https://stackoverflow.com/a/23699893/671658

@wjwwood
Copy link
Member

wjwwood commented Oct 8, 2019

We could use _x_ instead of __, which could also lead to collisions, but is unlikely to have a collision and is standard compliant.

@dirk-thomas dirk-thomas added the wontfix This will not be worked on label Dec 3, 2019
@dirk-thomas
Copy link
Member

We have discussed this in today's meeting and agree that the current use two consecutive underscores is undesired since it doesn't follow the spec.

  • We don't want to go back and follow the Google styleguide either and use a single underscore since the resulting ambiguity has actually bitten us in the past and resulted in colliding header guards from different files.

  • Considering the alternative to use any other kind of valid separator (e.g. _x_) while that would be compliant with the C++ spec there is still an unlikely chance of collision. So the solution is not significantly better than following the Google styleguide.

  • While pragma once works across all of our target platforms we understand the hesitation to use it since it is not covered by the spec either. Therefore we don't want to rely on it.

Since the current usage works (even though it doesn't comply with the C++ spec) and we are not aware of any actual problem caused by it we don't see a good enough alternative to switch to. As a consequence we don't plan to make a change to the header guards at the moment. Therefore I will mark the ticket with "wontfix" and go ahead and close it.

@elfring
Copy link
Author

elfring commented Dec 3, 2019

Since the current usage works (even though it doesn't comply with the C++ spec) and we are not aware of any actual problem caused by it we don't see a good enough alternative to switch to.

  • Would you like to restrict the safe application of your software to the implementation of C++ compilers?
  • Will you eventually care more for standard compliance?

@dirk-thomas
Copy link
Member

dirk-thomas commented Dec 3, 2019

I think I have described in details in my previous comment that we would like to have a standard compliant solution which doesn't have real-world drawbacks like collisions. So I am not sure why you are asking these questions.

@elfring Please feel free to make concrete proposals.

@elfring
Copy link
Author

elfring commented Dec 3, 2019

I found my suggestion for a better name selection concrete enough.

@dirk-thomas
Copy link
Member

I found my suggestion for a better name selection concrete enough.

Assuming you are referring to your one-line UUID reference please describe with a few more words how you imagine that to be done.

@elfring
Copy link
Author

elfring commented Dec 3, 2019

  • It would probably be simpler just to replace inappropriate double underscores.
  • UUIDs can be added also to include guards with the help of some tools.

@dirk-thomas
Copy link
Member

It would probably be simpler just to replace inappropriate double underscores.

The question is: with what to replace them? Please see my previous comment enumerating all the options we have considered.

UUIDs can be added also to include guards with the help of some tools.

Please be more concrete how exactly that process would look like on all the targeted platforms (Linux, macOS, Windows).

@elfring
Copy link
Author

elfring commented Dec 3, 2019

with what to replace them?

  • I imagine that a single underscore can be sufficient.
  • But you can choose also any other standard compliant solution.

Please be more concrete how exactly that process would look

I suggest to take just another look at known approaches to generate unique identifiers (also for include guards).

@dirk-thomas
Copy link
Member

  • I imagine that a single underscore can be sufficient.

Please read the above comments. That case has been explicitly discussed and the problem with that approach pointed out.

But you can choose also any other standard compliant solution.

Again: please make concrete proposals. Above we iterated over some proposed separators and also argued explicitly why we don't think they are a good solution.

@elfring
Copy link
Author

elfring commented Dec 3, 2019

If you do still not like a single underscore as a simple replacement, you can choose a more pleasing text instead.

@dirk-thomas
Copy link
Member

If you do still not like a single underscore as a simple replacement, you can choose a more pleasing text instead.

Your comments are not helpful and you are repeatedly ignoring feedback already given.

I am sorry but I won't be able to iterate with you on this (without any significant new input).

@elfring
Copy link
Author

elfring commented Dec 3, 2019

It seems to be easier for you to reject extra development efforts for achieving desirable C++ standard compliance.

@wjwwood
Copy link
Member

wjwwood commented Dec 4, 2019

How do you think about to use a kind of UUIDs for include guards?

I think UUIDs could be a solution we would adopt, but we would need some tools to help our developers and contributors when they are making new headers. Do you have recommendations on such tools?

Note, "use editor X" is probably not a good solution, since people will want to use their own editors and not be convinced to switch for such a narrow feature. Maybe there's a website that generates them or something?


If you do still not like a single underscore as a simple replacement, you can choose a more pleasing text instead.

@elfring I hope you can see why we feel that this isn't helpful feedback...

We did take time out of our weekly meeting to brainstorm ideas and didn't come up with anything better. If you also cannot come up with a better mechanism, then please stop suggesting we pick a different one. It doesn't move the issue forward.

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this issue Aug 5, 2022
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants