-
Notifications
You must be signed in to change notification settings - Fork 162
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
Pragmas #1811
Pragmas #1811
Conversation
Mhm. I think I hit parts of the scanner with |
I'll resolve the conflicts at least. I will also email gap@gap-system.org about this proposal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections to adding pragmas, as long as there is sufficient documentation (both on the C source code side, and, on the long run, in the manual). We can nitpick a bit about whether to use #@
or some other syntax, but that's an easy to resolve details.
The feature could potentially open up some very interesting capabilities, once the pragmas can be queries using some kind of reflection (e.g. via Markus' syntax tree work).
For the actual changes, I'd prefer if it did not mix code reformatting with a code change -- it makes it difficult to spot what the actual change is.
(Indeed, for this code in read.c
, I personally kinda prefer the compact, non-standard code formatting. For me, it helps to understand the code, and see more of it together, and the formatting has been helpful to spot the occasional problem quicker than would have been possible with what clang-format
produces. But of course that's a matter of taste. So if the majority prefers to reformat, fine, but then please do it in a dedicated separate commit.
src/read.c
Outdated
C_NEW_STRING( pragma, STATE(ValueLen), (void *)STATE(Value) ); | ||
len = STATE(ValueLen); | ||
SET_LEN_STRING(pragma, len); | ||
*(CHARS_STRING(pragma) + len) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C_NEW_STRING
and NEW_STRING
actually already call SET_LEN_STRING
, and add a zero terminator, so the previous three lines are not needed.
I have also noticed that the more compact form of that particular I wonder though what you mean by "separate commit"? Do you mean "separate pull request", because the reformatting is in fact in a separate commit (32567a3) |
4d2de0f
to
e729a4a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1811 +/- ##
==========================================
+ Coverage 65.44% 65.45% +<.01%
==========================================
Files 930 930
Lines 275830 275872 +42
Branches 12737 12709 -28
==========================================
+ Hits 180530 180564 +34
- Misses 92516 92520 +4
- Partials 2784 2788 +4
|
I would agree with trying not to clang-format too much (although sometimes However, I think we should over time aim to |
Re |
@markuspf while there is a separate commit for reformatting, not all reformatting is done in it: |
So, just before I start faffing with this again: What's the preferred formatting for
Any further wishes before this could go towards merging? |
I would prefer clang-format, just because I think we should move everything towards clang-format. Otherwise, I have to start debating every time I run clang-format if it's really better or not. |
into statement sequences. This, together with the syntaxtree module will enable users to process GAP source without writing their own ad-hoc parsers.
This PR introduces "pragmas" into the GAP language using the following syntax
Pragmas are currently stored as strings in the statement sequence (which means they can currently only usefully appear in function bodies, after
local
, its a minor change to be able to add them beforelocal
as well).The idea of pragmas is that we get a (even backwards compatible) way to annotate code with documentation, typing hints, or other things (like code for @frankluebeck's new Demo code).
Together with the syntax-tree module this could get us closer to not having to scrape GAP code manually for packages like
AutoDoc
, but currently we could introduce a functionGetPragmas(func)
to get the pragmas that are in the function object as a list of strings.We possibly want a mechanism to have namespaces for pragmas, so that packages can have their own names without clashing.
This PR serves the purpose to discuss