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

Support for "volatile" keyword #1667

Merged
merged 1 commit into from Jan 8, 2019
Merged

Support for "volatile" keyword #1667

merged 1 commit into from Jan 8, 2019

Conversation

jdemeyer
Copy link
Contributor

No description provided.

@jdemeyer jdemeyer force-pushed the volatile branch 3 times, most recently from 4d044ad to a77a58a Compare April 11, 2017 10:04
@scoder
Copy link
Contributor

scoder commented May 1, 2017

From a code design perspective, it feels like there should be two separate subclasses for the CConstOrVolatileType, even if most of the implementation is in a shared base class. Also, I'm sure I'll fail to remember that "cv" stands for "const or volatile" when I look at the code later. And I don't see a real need for a flag that merges the two - why not just test both flags in conditions that apply to both?

@scoder
Copy link
Contributor

scoder commented May 1, 2017

Thinking about this some more, I'm unsure if const and volatile are really the same kind of thing from Cython's perspective. They are both C modifiers, ok, and const is certainly part of the type, but is volatile also part of the type? It feels more like a part of a variable (declaration) than a part of a data type.

@jdemeyer
Copy link
Contributor Author

jdemeyer commented May 2, 2017

From a C/C++ point of view, "const" and "volatile" are often considered together. The name "cv" isn't my invention, it's regularly used when talking about const/volatile modifiers (just one example: http://en.cppreference.com/w/cpp/language/cv). But of course I can change cv to something else for Cython.

@jdemeyer
Copy link
Contributor Author

jdemeyer commented May 2, 2017

And I don't see a real need for a flag that merges the two - why not just test both flags in conditions that apply to both?

The problem is that in the current code, is_const is really used for two things: on the one hand, it is used for semantic const-ness (to prevent assignment for example). On the other hand, it is used as replacement for an isinstance(..., CConstType) check or a hasttr(... , "const_base_type") check. For the former, I am using is_const/is_volatile and for the latter I am using is_const_or_volatile.

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jan 1, 2019

Is there any chance to revisit this?

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Ok, I'm fine with this change. Only found a little improvement to simplify the parsing code.

@@ -2481,14 +2481,25 @@ def p_c_simple_base_type(s, self_flag, nonempty, templates = None):
pos = s.position()
if not s.sy == 'IDENT':
error(pos, "Expected an identifier, found '%s'" % s.sy)
if s.systring == 'const':
s.next()
if s.systring in ['const', 'volatile']:
Copy link
Contributor

Choose a reason for hiding this comment

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

Proposal:

is_const = is_volatile = False
while s.sy == 'IDENT' and s.systring in ('const', 'volatile'):
    ...
if s.sy != 'IDENT':
    error(...)
if is_const or is_volatile:
    ...

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jan 4, 2019

Fun fact: cdef const const int a is currently not an error, but I guess it should be :-)

@jdemeyer jdemeyer force-pushed the volatile branch 4 times, most recently from eef2a28 to 86a2a43 Compare January 8, 2019 13:36
@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jan 8, 2019

Why is Travis CI no longer testing this PR?

@scoder
Copy link
Contributor

scoder commented Jan 8, 2019

It is, its last test run was half an hour ago.

@scoder scoder merged commit 7d7e041 into cython:master Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants