Skip to content

Conversation

@mrange
Copy link
Contributor

@mrange mrange commented Jan 18, 2015

Origin: https://fslang.uservoice.com/forums/245727-f-language/suggestions/6079342-allow-extended-if-grammar

This commit adds the possibility to write condtional compilation expressions like this:
#if SILVERLIGHT || NETFX
#endif

or a bit more advanced:
#if (SILVERLIGHT || NETFX) && COMPILED || !DEBUG
#endif

This commit doesn't add #elif which is an orthogonal problem.

@mrange
Copy link
Contributor Author

mrange commented Jan 18, 2015

Migrated from CodePlex (mainly because I wanted a clean-slate as the history of this PR is quite long)

@mrange
Copy link
Contributor Author

mrange commented Jan 18, 2015

After pouring through the F# parser on how to do error handling I have incorporated similar techniques now. The result should be a more stable preprocessor parser and with support for better error messages to the user.

I ran the test suites (including VS integration)

I saw no errors that were due to extended if grammar.

There are two "small" issues remaining:

Inside visual studio the squiggle for unrecognized characters are rendered in the wrong place:
#if COMPILED # COMPILED // Squiggle rendered here -->_
#endif

The error message from the compiler looks correct though but I think its due to that I consume all characters. If I go back to a previous pattern and simpler pattern it's probably fixed.

The unit test for:
#if (COMPILED ||)
#endif

This generates a parse error (and indication the preprocessor parser needs to be extended to handle this case). I have code that are supposed to handle it but it doesn't pick it up properly yet.

Conclusion:

  1. I added Positive test cases on the fsc level which test various scenarios
  2. I added a new unit test container for internal compiler tests that test positive and negative scenarios
  3. The visual studio unit tests are extended with some new test cases
  4. I have tested the behavior om the VS addin in VS2013 and I think it behaves reasonable (with the exception of unrecognized chars)
  5. I have tested from the command line and think it behaves reasonable (expect for the parse error for certain incomplete expressions.

Therefore to my current knowledge I think this could be good to merge and schedule some minor bug fixes for later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect consuming "rest" is what causes the squiggle to be rendered in the wrong place on this error. Should be ok with just consuming '_'

@mexx
Copy link
Contributor

mexx commented Jan 18, 2015

Please consider to revert the changes in lexhelp.fs and lexhelp.fsi as they're only formatting and don't add any real value.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should not be included in the commit

@dsyme
Copy link
Contributor

dsyme commented Jan 20, 2015

I think the code looks good - the addition of FSharp.Compiler.UnitTests as well as the actual implementation of the feature.

I'm not sure if this has made the Jan 16 deadline for F# 4.0 though?

@latkin
Copy link
Contributor

latkin commented Jan 20, 2015

This has been in-progress and earmarked for F# 4.0 for a long time. Given Marten's responsiveness to my requests and feedback over the past weeks/months (see original Codeplex thread) I think it's only fair to merge this for 4.0.

@dsyme
Copy link
Contributor

dsyme commented Jan 20, 2015

Great, let's do it :)

@mrange
Copy link
Contributor Author

mrange commented Jan 20, 2015

FYI: Updated PR

  1. Removed generated pp* files (@dsyme feedback)
  2. Reverted lexhelp.* files (@mexx feedback)
  3. Fixed ExtendedIfGrammar.fs busted reporting of failures

Origin: https://fslang.uservoice.com/forums/245727-f-language/suggestions/6079342-allow-extended-if-grammar

This commit adds the possibility to write condtional compilation expressions like this:
 #if SILVERLIGHT || NETFX
 #endif

or a bit more advanced:
 #if (SILVERLIGHT || NETFX) && COMPILED || !DEBUG
 #endif

This commit doesn't add #elif which is an orthogonal problem.
@mrange mrange force-pushed the pr/extended_if_grammar branch from 417d3d1 to 42e0766 Compare January 20, 2015 18:11
@latkin latkin closed this in 438eae2 Jan 20, 2015
@latkin
Copy link
Contributor

latkin commented Jan 20, 2015

Applied! Fantastic, Marten. Thanks for your work on this over the past 3 months. 👏

if

@dsyme
Copy link
Contributor

dsyme commented Jan 21, 2015

Fantastic!!!

@dsyme
Copy link
Contributor

dsyme commented Jan 21, 2015

Hi @mrange,

We need to add a "Speclet" to this feature to this collection. The format should follow the others: Links, Aim, Background, Design, Specification and so on. The "specification" section should give a guide for how the F# Language Specification will be updated for this feature.

Could you submit a PR to that repo with an initial design/spec document please, and we'll iterate with you. If you need help doing the doc please let me know.

Thanks
Don

@mrange
Copy link
Contributor Author

mrange commented Jan 21, 2015

@dsyme - I threw together a speclet: fsharp/fslang-design#40

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.

4 participants