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

Add explicit flowtypes to ableC #60

Merged
merged 8 commits into from
Aug 28, 2017
Merged

Add explicit flowtypes to ableC #60

merged 8 commits into from
Aug 28, 2017

Conversation

krame505
Copy link
Member

This has been mostly straightforward, a few comments:

  • Pretty much every nonterminal should have an explicit flowtype for decorate
  • For attributes that are only used in 1 or 2 places I have written an flowtype at the declaration of those nonterminals
  • For attributes used generally (e.g. pp, errors, defs, host, lifted) I have written global per-nonterminal flowtype signatures either at the place where the attribute is declared or in Ast.sv for attributes from other grammars
  • The forward set I am also specifying in Ast.sv. For now this is empty for collection/wrapper nonterminals that we don't expect to extend (in order to allow pattern matching to be used without supplying the entire reference set), and to be the same as the reference set for all other nonterminals. This can change if we see a reason to do so.

For more easily writing flowtype declarations, I have been making use of the following script, make-flowtype-decl, that finds all nonterminals declared with an attribute and outputs a dummy declaration. Is this something worth adding the the Silver support scripts?

#!/bin/bash

attribute=$1
result=
for file in $(find . -name "*.sv")
do
    if [ -z attribute ]
    then
        result+='  '$(sed -n "N;s/.*nonterminal\\s*\([_a-zA-Z][_a-zA-Z0-9]*\).*/\1, /p;D" < $file | tr -d '\n')$'\n'
    else
        result+='  '$(sed -n "N;s/.*nonterminal\\s*\([_a-zA-Z][_a-zA-Z0-9]*\)\\s*with.*[^_a-zA-Z0-9]$attribute[^_a-zA-Z0-9].*/\1, /p;D" < $file | tr -d '\n')$'\n'
    fi
done

echo "flowtype $attribute {} on"
echo "$result" | sed '/^  $/d' | sed 's/ $//' | head -c-3
echo ";"

@krame505
Copy link
Member Author

@tedinski @ericvanwyk Any reason not to merge this once I get Jenkins to quit complaining?

@tedinski
Copy link
Member

lgtm

@ericvanwyk
Copy link
Contributor

Lucas,

This looks great.

You say "Pretty much every nonterminal should have an explicit flowtype for decorate" - so what are the exceptions to this rule?

Why not put the flow type info for a nonterminal at the declaration of the nonterminal instead of in Ast.sv? I don't think we should change it now, but I'm just curious.

After a week with SLE-type folks at Dagstuhl I've seen some nice editing envs that don't rely on sed scripts for creating this. I think you can commit this, but add a comment that point to the issue Ted created (Silver #188) about a better command line interface. We should compute these things semantically instead of with fragile syntactic things like this. I'm not criticizing - this is better than doing the work by hand. But it does point to a concern with Silver as it is now.

@krame505
Copy link
Member Author

The exceptions are 'enum-like' trivial nonterminals that have no inherited attributes, for which writing flowtypes seems silly - besides, we are considering replacing these with a Silver extension at some point anyway.

The flowtypes that I placed in Ast.sv or otherwise globally are ones that are fundamentally the same on every nonterminal, so repeating that information next to each nonterminal is much more painful and makes it harder to find the flowtypes for attributes specific to that nonterminal.

I agree that having Silver generate these would be way better, but writing a script was easier for the time being :-D I'll commit this when I have time.

@krame505 krame505 mentioned this pull request Aug 28, 2017
@krame505
Copy link
Member Author

I'm going to go ahead and merge this now...

@krame505 krame505 merged commit a5654ed into develop Aug 28, 2017
@krame505 krame505 deleted the feature/flowspecs branch October 24, 2017 11:52
@krame505 krame505 restored the feature/flowspecs branch October 12, 2018 06:54
@krame505 krame505 deleted the feature/flowspecs branch October 12, 2018 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants