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.

Sign up for GitHub

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

Jump to bottom

Expose own properties of scopes publicly #594

Merged
merged 2 commits into from
Sep 28, 2023
Merged

Expose own properties of scopes publicly #594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
297446a
Expose own properties of scopes publicly
sebastienros Sep 28, 2023
83ab044
Add aggressive inlining
sebastienros Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Fluid/Ast/ForStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,11 @@ public override async ValueTask<Completion> WriteToAsync(TextWriter writer, Text

var length = forloop.Length = startIndex + count;

context.LocalScope._properties["forloop"] = forloop;
context.LocalScope.SetOwnValue("forloop", forloop);

if (!parentLoop.IsNil())
{
context.LocalScope._properties["parentloop"] = parentLoop;
context.LocalScope.SetOwnValue("parentloop", parentLoop);
}

for (var i = startIndex; i < length; i++)
Expand All @@ -126,7 +126,7 @@ public override async ValueTask<Completion> WriteToAsync(TextWriter writer, Text

var item = source[i];

context.LocalScope._properties[Identifier] = item;
context.LocalScope.SetOwnValue(Identifier, item);

// Set helper variables
forloop.Index = i + 1;
Expand All @@ -150,7 +150,7 @@ public override async ValueTask<Completion> WriteToAsync(TextWriter writer, Text

//// Restore the forloop property after every statement in case it replaced it,
//// for instance if it contains a nested for loop
//context.LocalScope._properties["forloop"] = forloop;
//context.LocalScope.SetOwnValue("forloop", forloop);

if (completion != Completion.Normal)
{
Expand Down
52 changes: 40 additions & 12 deletions Fluid/Scope.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Fluid
{
public class Scope
{
internal Dictionary<string, FluidValue> _properties;
private Dictionary<string, FluidValue> _properties;
private readonly bool _forLoopScope;

public Scope()
Expand Down Expand Up @@ -66,41 +66,69 @@ public FluidValue GetValue(string name)
: NilValue.Instance;
}

/// <summary>
/// Deletes the value with the specified name in the chain of scopes.
/// </summary>
/// <param name="name">The name of the value to delete.</param>
public void Delete(string name)
{
if (!_forLoopScope)
{
DeleteOwn(name);
}
else
{
Parent.Delete(name);
}
}

/// <summary>
/// Deletes the value with the specified name in the current scopes.
/// </summary>
/// <param name="name">The name of the value to delete.</param>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void DeleteOwn(string name)
{
if (_properties != null)
{
if (!_forLoopScope)
{
_properties.Remove(name);
}
else
{
Parent.Delete(name);
}
_properties.Remove(name);
}
}

/// <summary>
/// Sets the value with the specified name in the chain of scopes.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void SetValue(string name, FluidValue value)
{
if (!_forLoopScope)
{
_properties ??= new Dictionary<string, FluidValue>();

_properties[name] = value ?? NilValue.Instance;
SetOwnValue(name, value);
}
else
{
Parent.SetValue(name, value);
}
}

/// <summary>
/// Sets the value with the specified name in the current scope.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void SetOwnValue(string name, FluidValue value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe [MethodImpl(MethodImplOptions.AggressiveInlining)]?

Copy link
Owner Author

Choose a reason for hiding this comment

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

What's your approach with that, I am tending to let the JIT decide these days because I don't have a benchmark that proves it better. But do you think it can never be worse than not having it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

JIT can help and be wise, but probably not on full framework. I made the comment mostly because you converted from direct field access to method so call site would more likely have old behavior with aggressive inlining.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point. I assume it doesn't hurt to add it whatsoever so I am adding it back.

{
_properties ??= new Dictionary<string, FluidValue>();
_properties[name] = value ?? NilValue.Instance;
}

public FluidValue GetIndex(FluidValue index)
{
return GetValue(index.ToString());
}

/// <summary>
/// Copies all the local scope properties to a different one.
/// </summary>
public void CopyTo(Scope scope)
{
if (_properties != null)
Expand Down