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 #45

Closed
elfring opened this issue Mar 26, 2016 · 12 comments
Closed

reserved identifier violation #45

elfring opened this issue Mar 26, 2016 · 12 comments
Assignees
Labels
type.Enhancement Issue requests feature type.Opinion
Milestone

Comments

@elfring
Copy link

elfring commented Mar 26, 2016

I would like to point out that identifiers like "_MyPaintSurface" and "_OperationQueue" do not fit to the expected naming convention of the C++ language standard.
Would you like to adjust your selection for unique names?

@odysseywestra
Copy link
Member

@elfring if you want, you can fork it and fix the identifiers and send us a pull request with the fix. That would be a great help.

@elfring
Copy link
Author

elfring commented Mar 27, 2016

Will a renaming of such identifiers affect also the application binary interface for this software?

@achadwick
Copy link
Member

libmypaint is written in C, and loosely conforms to the C99 standard. Why are you asking for this?

@achadwick
Copy link
Member

@elfring
We'll have to consider API consistency more carefully than we do in future. ABI consistency is less important IMO.

A bit more context would help us understand the issue you are having. What problems is are this naming causing you?
Otherwise, for now, I'm not sure if this is an issue we need to fix, frankly.

@elfring
Copy link
Author

elfring commented Mar 27, 2016

Why are you asking for this?

I imagine that more software release planning would be needed if you care for API/ABI stability around affected identifiers.

What problems is this naming causing you?

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

@achadwick
Copy link
Member

Hit me with practical suggestions about how we can improve things, tell me what's actually going wrong, or (better still) make PRs to fix dud behaviour please.

We can also consider automated tools as suggested in #46.

@achadwick achadwick added this to the v1.3.0 milestone May 25, 2016
@achadwick achadwick added the type.Enhancement Issue requests feature label May 25, 2016
@achadwick achadwick self-assigned this May 25, 2016
achadwick added a commit that referenced this issue May 25, 2016
Probably won't address #45 really, but I've seen a few
related linkage errors from MyPaint.
@achadwick
Copy link
Member

Still no practical suggestions or error messages from the reporter. Our codebase seems to be merely doing this to avoid confusing struct Foo and Foo types though, which is silly.

Picking this up for the release for another look. There are structures which probably should go private just for the sake of cleaning up the interface.

@elfring Tell me what problems this is causing you, or give me a concrete way of testing this. Compiler error messages, test cases, anything. Better still, send a PR.

@elfring
Copy link
Author

elfring commented May 25, 2016

Would you really like to "test" undefined behaviour? ;-)

@achadwick
Copy link
Member

@elfring Yes please. Suggestions for linters, compiler flags, or similar checks.

@elfring
Copy link
Author

elfring commented May 25, 2016

@achadwick
Copy link
Member

achadwick commented May 25, 2016

As if I haven't already, you mean?

Thanks for the link to the ubsan information on the RedHat blog. Looks as if it could be useful, but I note one of the comments on the post:

Most GNOME projects are maintained or contributed to by Red Hat and the use of reserved identifiers is pervasive among those projects. By pervasive, I mean close to 100% of GNOME projects do it and many of the examples in the documentation do too. It’s almost like some kind of style guide there.

Dunno if our use of GLib or GI will be a problem there.

achadwick added a commit to mypaint/mypaint that referenced this issue May 26, 2016
For consistency with mypaint/libmypaint#45.

Also use the proper header for defining that libmypaint symbol.
achadwick added a commit that referenced this issue May 27, 2016
Addresses #45. I don't want to turn this on except for
debug builds, just in case it has a performace hit.
@elfring
Copy link
Author

elfring commented May 27, 2016

Thanks for your source code improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type.Enhancement Issue requests feature type.Opinion
Development

No branches or pull requests

3 participants