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

Pragmas #1811

Closed
wants to merge 1 commit into from
Closed

Pragmas #1811

wants to merge 1 commit into from

Conversation

markuspf
Copy link
Member

This PR introduces "pragmas" into the GAP language using the following syntax

f := function(x)
   local y;
   #@ y : Int
end;

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 before local 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 function GetPragmas(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

  • Whether pragmas are considered useful, or whether we should come up with different approaches for the usecases we can come up with.
  • Whether there are objections

@markuspf markuspf added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) request-for-comments labels Oct 26, 2017
@markuspf markuspf self-assigned this Oct 26, 2017
@markuspf
Copy link
Member Author

Mhm. I think I hit parts of the scanner with clang-format-region. I am not sure whether that was a good idea (it is in general, but maybe not in this case?)

@markuspf
Copy link
Member Author

I'll resolve the conflicts at least. I will also email gap@gap-system.org about this proposal.

Copy link
Member

@fingolfin fingolfin left a 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;
Copy link
Member

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.

@markuspf
Copy link
Member Author

I have also noticed that the more compact form of that particular switch is better than clang-format, maybe I should then manually make it cleaner.

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)

@markuspf markuspf force-pushed the pragmas branch 3 times, most recently from 4d2de0f to e729a4a Compare October 26, 2017 18:14
@codecov
Copy link

codecov bot commented Oct 26, 2017

Codecov Report

Merging #1811 into master will increase coverage by <.01%.
The diff coverage is 84.09%.

@@            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
Impacted Files Coverage Δ
src/scanner.h 100% <ø> (ø) ⬆️
src/code.h 100% <ø> (ø) ⬆️
src/read.c 84.28% <100%> (+0.08%) ⬆️
src/code.c 89.92% <100%> (+0.04%) ⬆️
src/stats.c 78.64% <33.33%> (-0.51%) ⬇️
src/intrprtr.c 69.94% <50%> (-0.06%) ⬇️
src/scanner.c 78.46% <95.45%> (+0.7%) ⬆️
src/hpc/traverse.c 77.99% <0%> (-0.39%) ⬇️
hpcgap/lib/hpc/stdtasks.g 38.61% <0%> (-0.26%) ⬇️
... and 4 more

@ChrisJefferson
Copy link
Contributor

I would agree with trying not to clang-format too much (although sometimes git clang-format gets too eager and changes too much).

However, I think we should over time aim to clang-format everything, because the simplicity of saying to people "Just run clang-format and that's what code should look like" outweighs deciding if, for particular bits of code, that some other formatting is nicer (and of course, then we can disagree on what is "nicer" than clang-format)

@fingolfin
Copy link
Member

Re clang-format: Yeah, it's a bit of a conundrum... using clang-format on everything indeed is sweet and simple, and is great in 98% of the codebase, but really annoying in the other 2% (numbers are of course made up :-). We could use // clang-format off and // clang-format on on code for which we want to preserve custom formatting. Not that I really like this, but I can't think of anything better

@fingolfin
Copy link
Member

@markuspf while there is a separate commit for reformatting, not all reformatting is done in it: ReadStats is reformatted in the last commit, which is supposed to "only" add support for pragmas.

@markuspf
Copy link
Member Author

markuspf commented Nov 1, 2017

So, just before I start faffing with this again: What's the preferred formatting for GetSymbol and ReadStats? I still think the clang-formatted version is nicer, but I could go back to a slightly more terse formatting. Also, I will add a

  • GetFuncPragmas function (to get a list of pragmas in a function)
  • putting pragmas before local in a function body
  • some documentation

Any further wishes before this could go towards merging?

@ChrisJefferson
Copy link
Contributor

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.

@markuspf markuspf added this to the GAP 4.10.0 milestone Nov 3, 2017
into statement sequences.

This, together with the syntaxtree module will enable users to process
GAP source without writing their own ad-hoc parsers.
@fingolfin fingolfin removed this from the GAP 4.10.0 milestone Sep 28, 2018
@fingolfin fingolfin added kind: discussion discussions, questions, requests for comments, and so on and removed kind: request for comments labels Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: discussion discussions, questions, requests for comments, and so on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants