F# RFC FS-1015 - Support for "fixed" #1270

Merged
merged 18 commits into from Jun 27, 2016

Conversation

Projects
None yet
6 participants
@dsyme
Contributor

dsyme commented Jun 20, 2016

This is the PR for RFC FS-1015 - Support for 'fixed'

  • Complete implementation
  • Add tests
  • Fix baselines

Some tests are present but more are needed.

dsyme added some commits Jun 20, 2016

build.cmd
+if /i '%ARG%' == 'ci_part3' (
+ set BUILD_PROTO=1
+ set SKIP_EXPENSIVE_TESTS=1
+ set BUILD_CORECLR=1

This comment has been minimized.

@OmarTawfik

OmarTawfik Jun 21, 2016

Member

Why are we building CORECLR without testing it? shouldn't it be already covered in ci_part2?

@OmarTawfik

OmarTawfik Jun 21, 2016

Member

Why are we building CORECLR without testing it? shouldn't it be already covered in ci_part2?

dsyme added some commits Jun 21, 2016

@dsyme dsyme changed the title from [WIP] F# RFC FS-1015 - Support for "fixed" to F# RFC FS-1015 - Support for "fixed" Jun 21, 2016

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 21, 2016

Contributor

This is now ready for review and commit

Contributor

dsyme commented Jun 21, 2016

This is now ready for review and commit

src/fsharp/lexhelp.fs
@@ -284,7 +285,7 @@ module Keywords =
[ "atomic"; "break";
"checked"; "component"; "constraint"; "constructor"; "continue";
"eager";
- "fixed"; "fori"; "functor";

This comment has been minimized.

@vladima

vladima Jun 21, 2016

Contributor

yay! :)

@vladima

vladima Jun 21, 2016

Contributor

yay! :)

dsyme added some commits Jun 21, 2016

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 22, 2016

Contributor

OK, this is now green - please review!

Contributor

dsyme commented Jun 22, 2016

OK, this is now green - please review!

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 22, 2016

It looks like, for str a string, that both

use pStr = fixed str
use (pStr: nativeptr<char>) = fixed str

compile to the native int type in IL, whereas the C# fixed (char* pStr = str) compiles to the char* type in IL.

Not being an expert in how the CLR works, is this a cause for concern?

ghost commented Jun 22, 2016

It looks like, for str a string, that both

use pStr = fixed str
use (pStr: nativeptr<char>) = fixed str

compile to the native int type in IL, whereas the C# fixed (char* pStr = str) compiles to the char* type in IL.

Not being an expert in how the CLR works, is this a cause for concern?

src/absil/ilwrite.fs
+ locals |> ILList.iter (fun l ->
+ if l.IsPinned then
+ bb.EmitByte et_PINNED
+ EmitType cenv env bb l.Type)

This comment has been minimized.

@mexx

mexx Jun 23, 2016

Contributor

Can we please remove the duplication for this logic?
How about a new function EmitTypeOfLocal?

@mexx

mexx Jun 23, 2016

Contributor

Can we please remove the duplication for this logic?
How about a new function EmitTypeOfLocal?

This comment has been minimized.

@dsyme

dsyme Jun 23, 2016

Contributor

thanks, will do

@dsyme

dsyme Jun 23, 2016

Contributor

thanks, will do

src/fsharp/tast.fs
/// Get the flags as included in the F# binary metadata
member x.PickledBits =
// Clear the RecursiveValInfo, only used during inference and irrelevant across assembly boundaries
// Clear the IsCompiledAsStaticPropertyWithoutField, only used to determine whether to use a true field for a value, and to eliminate the optimization info for observable bindings
// Clear the HasBeenReferenced, only used to report "unreferenced variable" warnings and to help collect 'it' values in FSI.EXE
// Clear the IsGeneratedEventVal, since there's no use in propagating specialname information for generated add/remove event vals
- (flags &&& ~~~0b011001100000000000L)
+ (flags &&& ~~~0b1011001100000000000L)

This comment has been minimized.

@mexx

mexx Jun 23, 2016

Contributor

If I understand correctly we clear the bit for IsFixed, right?
Can we add a comment about the reasons as well?

@mexx

mexx Jun 23, 2016

Contributor

If I understand correctly we clear the bit for IsFixed, right?
Can we add a comment about the reasons as well?

This comment has been minimized.

@dsyme

dsyme Jun 23, 2016

Contributor

Good catch, Ii had the bit the wrong way around, it should be kept

@dsyme

dsyme Jun 23, 2016

Contributor

Good catch, Ii had the bit the wrong way around, it should be kept

@@ -431,7 +431,7 @@ let BindExternalLocalVal cenv (v:Val) vval env =
CheckInlineValueIsComplete v vval;
#endif
- if verboseOptimizationInfo then dprintn ("*** Binding "^v.LogicalName);
+ if verboseOptimizationInfo then dprintn ("*** Binding "+v.LogicalName);

This comment has been minimized.

@mexx

mexx Jun 23, 2016

Contributor

how is this change of operator is related to the feature?

@mexx

mexx Jun 23, 2016

Contributor

how is this change of operator is related to the feature?

This comment has been minimized.

@dsyme

dsyme Jun 23, 2016

Contributor

it's not, just cleanup. We're gradually being a bit more relaxed about that.

@dsyme

dsyme Jun 23, 2016

Contributor

it's not, just cleanup. We're gradually being a bit more relaxed about that.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 23, 2016

Contributor

@codymack As far as I understand things that's not a cause for concern. F# has always used nativeint for pointers, at least within method body code - but operationally there is no difference between the two.

Contributor

dsyme commented Jun 23, 2016

@codymack As far as I understand things that's not a cause for concern. F# has always used nativeint for pointers, at least within method body code - but operationally there is no difference between the two.

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Jun 24, 2016

Contributor

@dotnet-bot test this please

Contributor

KevinRansom commented Jun 24, 2016

@dotnet-bot test this please

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Jun 24, 2016

Contributor

@dsyme can you fix the merge issues

Contributor

KevinRansom commented Jun 24, 2016

@dsyme can you fix the merge issues

@KevinRansom KevinRansom merged commit 858eddf into Microsoft:master Jun 27, 2016

4 checks passed

Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment