ronantakizawa/codereview-bench huggingface dataset:
Repository: FlaxEngine/FlaxEngine
File: Source/Engine/Scripting/DotNet/CoreCLR.cpp (C++)

Code before:
void* CoreCLR::GetStaticMethodPointer(const String& methodName)
{
    void* fun;
    if (cachedFunctions.TryGet(methodName, fun))
        return fun;

    int rc = get_function_pointer(typeName, FLAX_CORECLR_STRING(methodName).Get(), UNMANAGEDCALLERSONLY_METHOD, nullptr, nullptr, &fun);
    if (rc != 0)
        LOG(Fatal, "Failed to get unmanaged function pointer for method {0}: 0x{1:x}", methodName.Get(), (unsigned int)rc);

    cachedFunctions.Add(String(methodName), fun);

    return fun;
}

void CoreCLR::RegisterNativeLibrary(const char* moduleName, const char* modulePath)
{
    static void* RegisterNativeLibraryPtr = CoreCLR::GetStaticMethodPointer(TEXT("RegisterNativeLibrary"));
    CoreCLR::CallStaticMethod<void, const char*, const char*>(RegisterNativeLibraryPtr, moduleName, modulePath);
}

void* CoreCLR::Allocate(int size)
{
    void* ptr = Platform::Allocate(size, 16);
#if COMPILE_WITH_PROFILER
    Platform::OnMemoryAlloc(ptr, size);
#endif
    return ptr;
}

void CoreCLR::Free(void* ptr)
{
#if COMPILE_WITH_PROFILER
    Platform::OnMemoryFree(ptr);
#endif
    Platform::Free(ptr);
}


Review comment (suggestion):
You can remove `OnMemoryAlloc`/`OnMemoryFree` calls when using `Platform::Allocate`/`Free` (it already calls profiler info)

Code after:
void* CoreCLR::GetStaticMethodPointer(const String& methodName)
{
    void* fun;
    if (cachedFunctions.TryGet(methodName, fun))
        return fun;

    int rc = get_function_pointer(typeName, FLAX_CORECLR_STRING(methodName).Get(), UNMANAGEDCALLERSONLY_METHOD, nullptr, nullptr, &fun);
    if (rc != 0)
        LOG(Fatal, "Failed to get unmanaged function pointer for method {0}: 0x{1:x}", methodName.Get(), (unsigned int)rc);

    cachedFunctions.Add(String(methodName), fun);

    return fun;
}

void CoreCLR::RegisterNativeLibrary(const char* moduleName, const char* modulePath)
{
    static void* RegisterNativeLibraryPtr = CoreCLR::GetStaticMethodPointer(TEXT("RegisterNativeLibrary"));
    CoreCLR::CallStaticMethod<void, const char*, const char*>(RegisterNativeLibraryPtr, moduleName, modulePath);
}

void* CoreCLR::Allocate(int size)
{
    return Platform::Allocate(size, 16);
}

void CoreCLR::Free(void* ptr)
{
    Platform::Free(ptr);
}
Repository: FlaxEngine/FlaxEngine
File: Source/Engine/Level/Tags.cpp (C++)

Code before:

void FLAXENGINE_API Serialization::Deserialize(ISerializable::DeserializeStream& stream, Tag& v, ISerializeModifier* modifier)
{
    v = Tags::Get(stream.GetText());
}

Tag Tags::Get(const StringView& tagName)
{
    if (tagName.IsEmpty())
        return Tag();
    Tag tag(List.Find(tagName) + 1);
    if (tag.Index == 0 && tagName.HasChars())
    {
        List.AddOne() = tagName;
        tag.Index = List.Count();
#if !BUILD_RELEASE
        TagsListDebug = List.Get();
#endif
    }
    return tag;
}

Array<Tag> Tags::GetSubTags(Tag parentTag)
{
    Array<Tag> subTags;
    auto _parentTagName = parentTag.ToString();

    for (int i = 0; i < Tags::List.Count(); i++)
    {
        auto& subTagName = Tags::List[i];
        if (subTagName.Contains(_parentTagName) && subTagName != _parentTagName)
        {
            subTags.Add(Tags::Get(subTagName));
        }
    }

    return subTags;
}

bool Tags::HasTag(const Array<Tag>& list, const Tag& tag)
{
    if (tag.Index == 0)
        return false;
    const String& tagName = tag.ToString();
    for (const Tag& e : list)
    {
        const String& eName = e.ToString();
        if (e == tag || (eName.Length() > tagName.Length() + 1 && eName.StartsWith(tagName) && eName[tagName.Length()] == '.'))
            return true;
    }
    return false;

Review comment (suggestion):
`const String&` instead of `auto` to reduce memory allocations

Code after:

void FLAXENGINE_API Serialization::Deserialize(ISerializable::DeserializeStream& stream, Tag& v, ISerializeModifier* modifier)
{
    v = Tags::Get(stream.GetText());
}

Tag Tags::Get(const StringView& tagName)
{
    if (tagName.IsEmpty())
        return Tag();
    Tag tag(List.Find(tagName) + 1);
    if (tag.Index == 0 && tagName.HasChars())
    {
        List.AddOne() = tagName;
        tag.Index = List.Count();
#if !BUILD_RELEASE
        TagsListDebug = List.Get();
#endif
    }
    return tag;
}

Array<Tag> Tags::GetSubTags(Tag parentTag)
{
    Array<Tag> subTags;
    const String& parentTagName = parentTag.ToString();

    for (int i = 0; i < Tags::List.Count(); i++)
    {
        const Tag tag = Tag(i + 1);
        const String& tagName = Tags::List[i];
        if (tagName.StartsWith(parentTagName) && parentTag.Index != tag.Index)
        {
            subTags.Add(tag);
        }
    }

    return subTags;
}

bool Tags::HasTag(const Array<Tag>& list, const Tag tag)
{
    if (tag.Index == 0)
        return false;
    const String& tagName = tag.ToString();
    for (const Tag& e : list)
    {
        const String& eName = e.ToString();
        if (e == tag || (eName.Length() > tagName.Length() + 1 && eName.StartsWith(tagName) && eName[tagName.Length()] == '.'))
            return true;
    }
Repository: FlaxEngine/FlaxEngine
File: Source/Engine/Level/Tags.cpp (C++)

Code before:
{
    if (tagName.IsEmpty())
        return Tag();
    Tag tag(List.Find(tagName) + 1);
    if (tag.Index == 0 && tagName.HasChars())
    {
        List.AddOne() = tagName;
        tag.Index = List.Count();
#if !BUILD_RELEASE
        TagsListDebug = List.Get();
#endif
    }
    return tag;
}

Array<Tag> Tags::GetSubTags(Tag parentTag)
{
    Array<Tag> subTags;
    auto _parentTagName = parentTag.ToString();

    for (int i = 0; i < Tags::List.Count(); i++)
    {
        auto& subTagName = Tags::List[i];
        if (subTagName.Contains(_parentTagName) && subTagName != _parentTagName)
        {
            subTags.Add(Tags::Get(subTagName));
        }
    }

    return subTags;
}

bool Tags::HasTag(const Array<Tag>& list, const Tag& tag)
{
    if (tag.Index == 0)
        return false;
    const String& tagName = tag.ToString();
    for (const Tag& e : list)
    {
        const String& eName = e.ToString();
        if (e == tag || (eName.Length() > tagName.Length() + 1 && eName.StartsWith(tagName) && eName[tagName.Length()] == '.'))
            return true;
    }
    return false;
}

bool Tags::HasTagExact(const Array<Tag>& list, const Tag& tag)
{
    if (tag.Index == 0)
        return false;
    return list.Contains(tag);

Review comment (suggestion):
`Tag(i + 1)`instead of `Tags::Get(subTagName)` as it's already in that tags list

Code after:
{
    if (tagName.IsEmpty())
        return Tag();
    Tag tag(List.Find(tagName) + 1);
    if (tag.Index == 0 && tagName.HasChars())
    {
        List.AddOne() = tagName;
        tag.Index = List.Count();
#if !BUILD_RELEASE
        TagsListDebug = List.Get();
#endif
    }
    return tag;
}

Array<Tag> Tags::GetSubTags(Tag parentTag)
{
    Array<Tag> subTags;
    const String& parentTagName = parentTag.ToString();

    for (int i = 0; i < Tags::List.Count(); i++)
    {
        const Tag tag = Tag(i + 1);
        const String& tagName = Tags::List[i];
        if (tagName.StartsWith(parentTagName) && parentTag.Index != tag.Index)
        {
            subTags.Add(tag);
        }
    }

    return subTags;
}

bool Tags::HasTag(const Array<Tag>& list, const Tag tag)
{
    if (tag.Index == 0)
        return false;
    const String& tagName = tag.ToString();
    for (const Tag& e : list)
    {
        const String& eName = e.ToString();
        if (e == tag || (eName.Length() > tagName.Length() + 1 && eName.StartsWith(tagName) && eName[tagName.Length()] == '.'))
            return true;
    }
    return false;
}

bool Tags::HasTagExact(const Array<Tag>& list, const Tag tag)
{
    if (tag.Index == 0)
        return false;
Repository: FlaxEngine/FlaxEngine
File: Source/Engine/Level/Tags.cpp (C++)

Code before:

Tag Tags::Get(const StringView& tagName)
{
    if (tagName.IsEmpty())
        return Tag();
    Tag tag(List.Find(tagName) + 1);
    if (tag.Index == 0 && tagName.HasChars())
    {
        List.AddOne() = tagName;
        tag.Index = List.Count();
#if !BUILD_RELEASE
        TagsListDebug = List.Get();
#endif
    }
    return tag;
}

Array<Tag> Tags::GetSubTags(Tag parentTag)
{
    Array<Tag> subTags;
    auto _parentTagName = parentTag.ToString();

    for (int i = 0; i < Tags::List.Count(); i++)
    {
        auto& subTagName = Tags::List[i];
        if (subTagName.Contains(_parentTagName) && subTagName != _parentTagName)
        {
            subTags.Add(Tags::Get(subTagName));
        }
    }

    return subTags;
}

bool Tags::HasTag(const Array<Tag>& list, const Tag& tag)
{
    if (tag.Index == 0)
        return false;
    const String& tagName = tag.ToString();
    for (const Tag& e : list)
    {
        const String& eName = e.ToString();
        if (e == tag || (eName.Length() > tagName.Length() + 1 && eName.StartsWith(tagName) && eName[tagName.Length()] == '.'))
            return true;
    }
    return false;
}

bool Tags::HasTagExact(const Array<Tag>& list, const Tag& tag)
{
    if (tag.Index == 0)

Review comment (suggestion):
It should be `subTagName.StartsWith()` as tag can contain other tags inside and not be a parent. Also, instead of comparing strings you can compare current tag index (`i+1`) with `parentTag.Index` for faster perf.

Code after:

Tag Tags::Get(const StringView& tagName)
{
    if (tagName.IsEmpty())
        return Tag();
    Tag tag(List.Find(tagName) + 1);
    if (tag.Index == 0 && tagName.HasChars())
    {
        List.AddOne() = tagName;
        tag.Index = List.Count();
#if !BUILD_RELEASE
        TagsListDebug = List.Get();
#endif
    }
    return tag;
}

Array<Tag> Tags::GetSubTags(Tag parentTag)
{
    Array<Tag> subTags;
    const String& parentTagName = parentTag.ToString();

    for (int i = 0; i < Tags::List.Count(); i++)
    {
        const Tag tag = Tag(i + 1);
        const String& tagName = Tags::List[i];
        if (tagName.StartsWith(parentTagName) && parentTag.Index != tag.Index)
        {
            subTags.Add(tag);
        }
    }

    return subTags;
}

bool Tags::HasTag(const Array<Tag>& list, const Tag tag)
{
    if (tag.Index == 0)
        return false;
    const String& tagName = tag.ToString();
    for (const Tag& e : list)
    {
        const String& eName = e.ToString();
        if (e == tag || (eName.Length() > tagName.Length() + 1 && eName.StartsWith(tagName) && eName[tagName.Length()] == '.'))
            return true;
    }
    return false;
}

bool Tags::HasTagExact(const Array<Tag>& list, const Tag tag)
{
Repository: FlaxEngine/FlaxEngine
File: Source/Editor/Windows/SceneTreeWindow.cs (C#)

Code before:
            base.OnDestroy();
        }
    }

    /// <summary>
    /// Action to perform when a Scene Node receive a double mouse click (Left)
    /// </summary>
    [Serializable]
    public enum SceneNodeDoubleClick
    {
        /// <summary>
        /// No action is performed
        /// </summary>
        None, 
        /// <summary>
        /// Rename the Scene Node
        /// </summary>
        Rename,
        /// <summary>
        /// Focus the Scene Node object in the Scene View
        /// </summary>
        Focus,
        /// <summary>
        /// If possible, open the scene node in an associated Editor (e.g. Prefab Editor)
        /// </summary>
        Open
    }
}


Review comment (suggestion):
It may be good to make these more descriptive. Like `ActorFocus`, `PrefabOpen`, `ActorRename`.

Code after:
            base.OnDestroy();
        }
    }

    /// <summary>
    /// Action to perform when a Scene Node receive a double mouse click (Left)
    /// </summary>
    [Serializable]
    public enum SceneNodeDoubleClick
    {
        /// <summary>
        /// No action is performed
        /// </summary>
        None, 
        /// <summary>
        /// Rename the Scene Node
        /// </summary>
        RenameActor,
        /// <summary>
        /// Focus the Scene Node object in the Scene View
        /// </summary>
        FocusActor,
        /// <summary>
        /// If possible, open the scene node in an associated Editor (e.g. Prefab Editor)
        /// </summary>
        OpenPrefab
    }
}
Repository: FlaxEngine/FlaxEngine
File: Source/Editor/Surface/IParametersDependantNode.cs (C#)

Code before:

        /// <summary>
        /// On parameter renamed.
        /// </summary>
        /// <param name="param">The parameter.</param>
        void OnParamRenamed(SurfaceParameter param);

        /// <summary>
        /// On parameter modified (eg. type changed).
        /// </summary>
        /// <param name="param">The parameter.</param>
        void OnParamEdited(SurfaceParameter param);

        /// <summary>
        /// On parameter deleted.
        /// </summary>
        /// <param name="param">The parameter.</param>
        void OnParamDeleted(SurfaceParameter param);

        /// <summary>
        /// Get if the parameter is referenced in a graph. Referenced in this case means in a graph and at least one node in-/output connected to another node.
        /// </summary>
        /// <param name="param">The parameter.</param>
        /// <param name="surface">The visject surface.</param>
        /// <returns>If the parameter is referenced.</returns>
        bool IsParamreferenced(SurfaceParameter param, VisjectSurface surface);
    }
}


Review comment (suggestion):
Drop `VisjectSurface surface` parameter as node itself knows about internals and can detect whether uses that param.

Code after:

        /// <summary>
        /// On parameter renamed.
        /// </summary>
        /// <param name="param">The parameter.</param>
        void OnParamRenamed(SurfaceParameter param);

        /// <summary>
        /// On parameter modified (eg. type changed).
        /// </summary>
        /// <param name="param">The parameter.</param>
        void OnParamEdited(SurfaceParameter param);

        /// <summary>
        /// On parameter deleted.
        /// </summary>
        /// <param name="param">The parameter.</param>
        void OnParamDeleted(SurfaceParameter param);

        /// <summary>
        /// Get if the parameter is referenced in a graph. Referenced in this case means in a graph and at least one node in-/output connected to another node.
        /// </summary>
        /// <param name="param">The parameter.</param>
        /// <returns>If the parameter is referenced.</returns>
        bool IsParamreferenced(SurfaceParameter param);
    }
}
Repository: FlaxEngine/FlaxEngine
File: Source/Editor/Surface/Archetypes/Parameters.cs (C#)

Code before:
            public void OnParamRenamed(SurfaceParameter param)
            {
                UpdateCombo();
                UpdateUI();
            }

            /// <inheritdoc />
            public void OnParamEdited(SurfaceParameter param)
            {
                UpdateUI();
            }

            /// <inheritdoc />
            public void OnParamDeleted(SurfaceParameter param)
            {
                // Deselect if that parameter is selected
                if ((Guid)Values[0] == param.ID)
                    _combobox.SelectedIndex = -1;

                UpdateCombo();
            }

            /// <inheritdoc />
            public bool IsParamreferenced(SurfaceParameter param, VisjectSurface surface)
            {
                for (int i = 0; i < surface.Nodes.Count; i++)
                {
                    if (surface.Nodes[i] is IParametersDependantNode node)
                    {
                        return (Guid)surface.Nodes[i].Values[0] == param.ID && surface.Nodes[i].GetBoxes().Any(b => b.Connections.Count > 0);
                    }
                }

                return false;
            }

            /// <inheritdoc />
            public override void OnLoaded(SurfaceNodeActions action)
            {
                base.OnLoaded(action);

                if (Surface != null)
                    UpdateCombo();
                UpdateUI();
            }

            /// <inheritdoc />
            public override void OnSurfaceLoaded(SurfaceNodeActions action)
            {
                base.OnSurfaceLoaded(action);


Review comment (refactor):
Simplify this code into `return (Guid)Values[0] == param.ID;`.

Code after:
            {
                UpdateUI();
            }

            /// <inheritdoc />
            public void OnParamDeleted(SurfaceParameter param)
            {
                // Deselect if that parameter is selected
                if ((Guid)Values[0] == param.ID)
                    _combobox.SelectedIndex = -1;

                UpdateCombo();
            }

            /// <inheritdoc />
            public bool IsParamreferenced(SurfaceParameter param)
            {
                return (Guid)Values[0] == param.ID;
            }

            /// <inheritdoc />
            public override void OnLoaded(SurfaceNodeActions action)
            {
                base.OnLoaded(action);

                if (Surface != null)
                    UpdateCombo();
                UpdateUI();
            }

            /// <inheritdoc />
            public override void OnSurfaceLoaded(SurfaceNodeActions action)
            {
                base.OnSurfaceLoaded(action);

                if (Surface != null)
                {
                    UpdateUI();
                }
            }

            /// <inheritdoc />
            public override void OnValuesChanged()
            {
                base.OnValuesChanged();

                UpdateCombo();
                UpdateUI();
            }

            private void UpdateUI()
Repository: FlaxEngine/FlaxEngine
File: Source/Editor/Surface/Archetypes/Parameters.cs (C#)

Code before:
            public void OnParamRenamed(SurfaceParameter param)
            {
                UpdateCombo();
                UpdateTitle();
            }

            /// <inheritdoc />
            public void OnParamEdited(SurfaceParameter param)
            {
                UpdateLayout();
            }

            /// <inheritdoc />
            public void OnParamDeleted(SurfaceParameter param)
            {
                // Deselect if that parameter is selected
                if ((Guid)Values[0] == param.ID)
                    _combobox.SelectedIndex = -1;

                UpdateCombo();
            }

            /// <inheritdoc />
            public bool IsParamreferenced(SurfaceParameter param)
            {
                for (int i = 0; i < Surface.Nodes.Count; i++)
                {
                    if (Surface.Nodes[i] is IParametersDependantNode node)
                    {
                        return (Guid)Values[0] == param.ID;
                    }
                }

                return false;
            }

            /// <inheritdoc />
            public override void OnLoaded(SurfaceNodeActions action)
            {
                base.OnLoaded(action);

                if (Surface != null)
                {
                    UpdateCombo();
                    UpdateLayout();
                }
            }

            /// <inheritdoc />
            public override void OnSurfaceLoaded(SurfaceNodeActions action)
            {

Review comment (suggestion):
This loop can be removed and whole function body can be:
```cs
return (Guid)Values[0] == param.ID;
```

Code after:
            public void OnParamRenamed(SurfaceParameter param)
            {
                UpdateCombo();
                UpdateTitle();
            }

            /// <inheritdoc />
            public void OnParamEdited(SurfaceParameter param)
            {
                UpdateLayout();
            }

            /// <inheritdoc />
            public void OnParamDeleted(SurfaceParameter param)
            {
                // Deselect if that parameter is selected
                if ((Guid)Values[0] == param.ID)
                    _combobox.SelectedIndex = -1;

                UpdateCombo();
            }

            /// <inheritdoc />
            public bool IsParamreferenced(SurfaceParameter param)
            {
                return (Guid)Values[0] == param.ID;
            }

            /// <inheritdoc />
            public override void OnLoaded(SurfaceNodeActions action)
            {
                base.OnLoaded(action);

                if (Surface != null)
                {
                    UpdateCombo();
                    UpdateLayout();
                }
            }

            /// <inheritdoc />
            public override void OnSurfaceLoaded(SurfaceNodeActions action)
            {
                base.OnSurfaceLoaded(action);

                UpdateTitle();
            }

            /// <inheritdoc />
            public override void OnValuesChanged()
            {
Repository: FlaxEngine/FlaxEngine
File: Source/Editor/Surface/Archetypes/Parameters.cs (C#)

Code before:
            public void OnParamRenamed(SurfaceParameter param)
            {
                UpdateCombo();
                UpdateUI();
            }

            /// <inheritdoc />
            public void OnParamEdited(SurfaceParameter param)
            {
                UpdateUI();
            }

            /// <inheritdoc />
            public void OnParamDeleted(SurfaceParameter param)
            {
                // Deselect if that parameter is selected
                if ((Guid)Values[0] == param.ID)
                    _combobox.SelectedIndex = -1;

                UpdateCombo();
            }

            /// <inheritdoc />
            public bool IsParamreferenced(SurfaceParameter param)
            {
                for (int i = 0; i < Surface.Nodes.Count; i++)
                {
                    if (Surface.Nodes[i] is IParametersDependantNode node)
                    {
                        return (Guid)Values[0] == param.ID;
                    }
                }

                return false;
            }

            /// <inheritdoc />
            public override void OnLoaded(SurfaceNodeActions action)
            {
                base.OnLoaded(action);

                if (Surface != null)
                    UpdateCombo();
                UpdateUI();
            }

            /// <inheritdoc />
            public override void OnSurfaceLoaded(SurfaceNodeActions action)
            {
                base.OnSurfaceLoaded(action);


Review comment (suggestion):
This loop can be removed and whole function body can be:
```cs
return (Guid)Values[0] == param.ID;
```

Code after:
            {
                UpdateUI();
            }

            /// <inheritdoc />
            public void OnParamDeleted(SurfaceParameter param)
            {
                // Deselect if that parameter is selected
                if ((Guid)Values[0] == param.ID)
                    _combobox.SelectedIndex = -1;

                UpdateCombo();
            }

            /// <inheritdoc />
            public bool IsParamreferenced(SurfaceParameter param)
            {
                return (Guid)Values[0] == param.ID;
            }

            /// <inheritdoc />
            public override void OnLoaded(SurfaceNodeActions action)
            {
                base.OnLoaded(action);

                if (Surface != null)
                    UpdateCombo();
                UpdateUI();
            }

            /// <inheritdoc />
            public override void OnSurfaceLoaded(SurfaceNodeActions action)
            {
                base.OnSurfaceLoaded(action);

                if (Surface != null)
                {
                    UpdateUI();
                }
            }

            /// <inheritdoc />
            public override void OnValuesChanged()
            {
                base.OnValuesChanged();

                UpdateCombo();
                UpdateUI();
            }

            private void UpdateUI()