Skip to content

Conversation

@joy999
Copy link
Contributor

@joy999 joy999 commented Nov 21, 2025

add the generic list map: ListKVMap[K,V] and let ListMap base on it.

@joy999 joy999 added the feature label Nov 21, 2025
@joy999 joy999 changed the title add generic list map feature feat(container/gmap): add generic list map feature Nov 21, 2025
@joy999 joy999 requested a review from Copilot November 21, 2025 09:03
Copilot finished reviewing on behalf of joy999 November 21, 2025 09:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new generic list map implementation (ListKVMap[K, V]) and refactors the existing ListMap to use it as the underlying implementation. The main goal is to provide a type-safe, generic version of the ordered map while maintaining backward compatibility with the existing non-generic ListMap.

  • Adds new generic ListKVMap[K, V] type with full map operations
  • Refactors ListMap to wrap ListKVMap[any, any] with lazy initialization
  • Delegates most ListMap methods to the underlying generic implementation

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.

File Description
container/gmap/gmap_list_k_v_map.go New file implementing the generic ListKVMap[K, V] type with type-safe operations for ordered key-value storage
container/gmap/gmap_list_map.go Refactored to use ListKVMap[any, any] as the underlying implementation with lazy initialization and method delegation
Comments suppressed due to low confidence (2)

container/gmap/gmap_list_map.go:319

  • Direct access to m.mu, m.data, and m.list bypasses the generic ListKVMap implementation after lazy initialization. This should use m.ListKVMap.UnmarshalValue(value) instead to maintain consistency with the new design.
	m.mu.Lock()
	defer m.mu.Unlock()

	for k, v := range gconv.Map(value) {
		if e, ok := m.data[k]; !ok {
			m.data[k] = m.list.PushBack(&gListMapNode{k, v})
		} else {
			e.Value = &gListMapNode{k, v}
		}
	}

container/gmap/gmap_list_map.go:279

  • The Flip() method signature is inconsistent with the underlying ListKVMap.Flip() method, which returns an error. This method should return error and propagate any errors from the conversion operations, or handle them appropriately. The current implementation silently ignores potential errors.
// Flip exchanges key-value of the map to value-key.
func (m *ListMap) Flip() {
	data := m.Map()
	m.Clear()
	for key, value := range data {
		m.Set(value, key)
	}
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hailaz hailaz requested a review from Copilot November 29, 2025 12:09
Copilot finished reviewing on behalf of hailaz November 29, 2025 12:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

container/gmap/gmap_list_map.go:320

  • The UnmarshalValue method is directly accessing m.data and m.list fields which are now private fields of the embedded ListKVMap[any, any] type. This will cause a compilation error. The method should delegate to m.ListKVMap.UnmarshalValue(value) instead of manually accessing the internal fields.
	m.mu.Lock()
	defer m.mu.Unlock()

	for k, v := range gconv.Map(value) {
		if e, ok := m.data[k]; !ok {
			m.data[k] = m.list.PushBack(&gListMapNode{k, v})
		} else {
			e.Value = &gListMapNode{k, v}
		}
	}
	return

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…保线程安全

test(container/gmap): 更新测试用例以验证空映射的行为
@hailaz hailaz merged commit 5a67aac into master Nov 29, 2025
20 checks passed
@hailaz hailaz deleted the feat/list-k-v-map branch November 29, 2025 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants