Repository: intel/rohd (dart)

Issue:
Add an easier way to name important generated signals
### Motivation

Sometimes the result of some hardware is important and needs to be named so that it can be visible in the generated output (e.g. SystemVerilog).  One way to do that is to first construct a named signal, then assign it to the result of that operation.  For example:

```dart
final myImportantSignal = Logic(name: 'my_sig')..gets(a & b | c);
```

This is a little verbose and the cascade notation can be intimidating/unintuitive to people new to Dart/ROHD.  An improved API for the same thing could look something like:

```dart
final myImportantSignal = (a & b | c).named('my_sig');
```


### Desired solution

Add a `named` function that provides an ability to construct a new `Logic` clone with a specified name (and `Naming`).  Perhaps adding a `clone` function to `Logic` (as already exists on `LogicStructure`) would be a good implementation approach, then `named` could just wrap `clone` with an even simpler API.

### Alternatives considered

_No response_

### Additional details

_No response_

Pull request description:
<!-- Please make sure you check out the contribution guidelines before submitting a pull request! -->

## Description & Motivation

The main things in this PR are:
- adding `Logic.clone`, which behaves similarly to the existing ones for `LogicStructure`
- adding `Logic.named`, which automates the creation of a named clone of a signal (usually for the purpose of making generated outputs prettier)

## Related Issue(s)

N/A

## Testing

Added new tests

## Backwards-compatibility

> Is this a breaking change that will not be backwards-compatible? If yes, how so?

No

## Documentation

> Does the change require any updates to documentation? If so, where? Are they included?

Not really, API docs cover it, maybe some general examples and guidance would be nice in the future

Patch:
diff --git a/lib/src/signals/logic.dart b/lib/src/signals/logic.dart
index ba07d68e3..004b5ba22 100644
--- a/lib/src/signals/logic.dart
+++ b/lib/src/signals/logic.dart
@@ -255,6 +255,35 @@ class Logic {
           naming: naming,
         );
 
+  /// A cloning utility for [clone] and [named].
+  Logic _clone({String? name, Naming? naming}) =>
+      (isNet ? LogicNet.new : Logic.new)(
+          name: name ?? this.name,
+          naming: Naming.chooseCloneNaming(
+              originalName: this.name,
+              newName: name,
+              originalNaming: this.naming,
+              newNaming: naming),
+          width: width);
+
+  /// Makes a copy of `this`, optionally with the specified [name], but the same
+  /// [width].
+  Logic clone({String? name}) => _clone(name: name);
+
+  /// Makes a [clone] with the provided [name] and optionally [naming], then
+  /// assigns it to be driven by `this`.
+  ///
+  /// This is a useful utility for naming the result of some hardware
+  /// construction without separately declaring a new named signal and then
+  /// assigning.  For example:
+  ///
+  /// ```dart
+  /// // named "myImportantNode" instead of a generated name like "a_xor_b"
+  /// final myImportantNode = (a ^ b).named('myImportantNode');
+  /// ```
+  Logic named(String name, {Naming? naming}) =>
+      _clone(name: name, naming: naming)..gets(this);
+
   /// An internal constructor for [Logic] which additional provides access to
   /// setting the [wire].
   Logic._({
diff --git a/lib/src/signals/logic_array.dart b/lib/src/signals/logic_array.dart
index 897c73aae..752350abf 100644
--- a/lib/src/signals/logic_array.dart
+++ b/lib/src/signals/logic_array.dart
@@ -137,8 +137,7 @@ class LogicArray extends LogicStructure {
     // calculate the next layer's dimensions
     final nextDimensions = dimensions.length == 1
         ? null
-        : UnmodifiableListView(
-            dimensions.getRange(1, dimensions.length).toList(growable: false));
+        : List<int>.unmodifiable(dimensions.getRange(1, dimensions.length));
 
     // if the total width will eventually be 0, then force element width to 0
     if (elementWidth != 0 && dimensions.reduce((a, b) => a * b) == 0) {
@@ -168,7 +167,7 @@ class LogicArray extends LogicStructure {
                 ))
             .._arrayIndex = index,
           growable: false),
-      dimensions: UnmodifiableListView(dimensions),
+      dimensions: List<int>.unmodifiable(dimensions),
       elementWidth: elementWidth,
       numUnpackedDimensions: numUnpackedDimensions,
       name: name,
@@ -177,13 +176,38 @@ class LogicArray extends LogicStructure {
     );
   }
 
+  @override
+  LogicArray _clone({String? name, Naming? naming}) => LogicArray._factory(
+        dimensions,
+        elementWidth,
+        name: name ?? this.name,
+        numUnpackedDimensions: numUnpackedDimensions,
+        naming: Naming.chooseCloneNaming(
+            originalName: this.name,
+            newName: name,
+            originalNaming: this.naming,
+            newNaming: naming),
+        logicBuilder: isNet ? LogicNet.new : Logic.new,
+        logicArrayBuilder: isNet ? LogicArray.net : LogicArray.new,
+        isNet: isNet,
+      );
+
   /// Creates a new [LogicArray] which has the same [dimensions],
-  /// [elementWidth], [numUnpackedDimensions] as `this`.
+  /// [elementWidth], [numUnpackedDimensions], and [isNet] as `this`.
   ///
   /// If no new [name] is specified, then it will also have the same name.
   @override
-  LogicArray clone({String? name}) => LogicArray(dimensions, elementWidth,
-      numUnpackedDimensions: numUnpackedDimensions, name: name ?? this.name);
+  LogicArray clone({String? name}) => _clone(name: name);
+
+  /// Makes a [clone] with the provided [name] and optionally [naming], then
+  /// assigns it to be driven by `this`.
+  ///
+  /// This is a useful utility for naming the result of some hardware
+  /// construction without separately declaring a new named signal and then
+  /// assigning.
+  @override
+  LogicArray named(String name, {Naming? naming}) =>
+      _clone(name: name, naming: naming)..gets(this);
 
   /// Private constructor for the factory [LogicArray] constructor.
   ///
diff --git a/lib/src/signals/logic_structure.dart b/lib/src/signals/logic_structure.dart
index e2b64aee6..10a0275a0 100644
--- a/lib/src/signals/logic_structure.dart
+++ b/lib/src/signals/logic_structure.dart
@@ -53,12 +53,29 @@ class LogicStructure implements Logic {
       });
   }
 
-  /// Creates a new [LogicStructure] with the same structure as `this`.
-  LogicStructure clone({String? name}) => LogicStructure(
-      elements.map((e) => e is LogicStructure
-          ? e.clone()
-          : Logic(name: e.name, width: e.width, naming: e.naming)),
-      name: name ?? this.name);
+  @override
+  LogicStructure _clone({String? name, Naming? naming}) =>
+      // naming is not used for LogicStructure
+      LogicStructure(elements.map((e) => e.clone(name: e.name)),
+          name: name ?? this.name);
+
+  /// Creates a new [LogicStructure] with the same structure as `this` and
+  /// [clone]d [elements], optionally with the provided [name].
+  @override
+  LogicStructure clone({String? name}) => _clone(name: name);
+
+  /// Makes a [clone], optionally with the specified [name], then assigns it to
+  /// be driven by `this`.
+  ///
+  /// The [naming] argument will not have any effect on a generic
+  /// [LogicStructure], but behavior may be overridden by implementers.
+  ///
+  /// This is a useful utility for naming the result of some hardware
+  /// construction without separately declaring a new named signal and then
+  /// assigning.
+  @override
+  LogicStructure named(String name, {Naming? naming}) =>
+      clone(name: name)..gets(this);
 
   @override
   String get structureName {
diff --git a/lib/src/utilities/naming.dart b/lib/src/utilities/naming.dart
index 260947661..e2b6e74cc 100644
--- a/lib/src/utilities/naming.dart
+++ b/lib/src/utilities/naming.dart
@@ -77,6 +77,29 @@ enum Naming {
               : Naming.renameable
           : Naming.unnamed);
 
+  /// Picks a [Naming] for a clone based on its original conditions and
+  /// optionally provided new conditions.
+  static Naming chooseCloneNaming({
+    required String originalName,
+    required String? newName,
+    required Naming originalNaming,
+    required Naming? newNaming,
+  }) {
+    if (newNaming != null) {
+      // if provided, then use that
+      return newNaming;
+    }
+
+    if (newName == null && newNaming == null) {
+      // if not provided, we can default to mergeable, since we clone the old
+      // name and don't necessarily need the duplicate around
+      return Naming.mergeable;
+    }
+
+    // otherwise, use default
+    return Naming.chooseNaming(newName, newNaming);
+  }
+
   /// Picks a [String] name based on an initial [name] and [naming].
   ///
   /// If [name] is null, the name will be based on [nullStarter].


Test patch:
diff --git a/test/logic_array_test.dart b/test/logic_array_test.dart
index 9d2689c65..aacfad8e9 100644
--- a/test/logic_array_test.dart
+++ b/test/logic_array_test.dart
@@ -957,12 +957,11 @@ void main() {
       final mod = WithSetArrayOffsetModule(LogicArray([2, 2], 8));
       await testArrayPassthrough(mod, checkNoSwizzle: false);
 
+      final sv = mod.generateSynth();
+
       // make sure we're reassigning both times it overlaps!
       expect(
-          RegExp('assign laIn.*=.*swizzled')
-              .allMatches(mod.generateSynth())
-              .length,
-          2);
+          RegExp(r'assign laOut\[1\].*=.*swizzled').allMatches(sv).length, 2);
     });
   });
 
@@ -1078,4 +1077,33 @@ void main() {
       SimCompare.checkIverilogVector(mod, vectors);
     });
   });
+
+  group('array clone', () {
+    for (final isNet in [true, false]) {
+      test('isNet = $isNet', () {
+        final la = (isNet ? LogicArray.net : LogicArray.new)(
+          [3, 2, 4],
+          8,
+          numUnpackedDimensions: 1,
+          name: 'myarray',
+          naming: Naming.reserved,
+        );
+        final clone = la.clone();
+        expect(la.dimensions, clone.dimensions);
+        expect(la.elementWidth, clone.elementWidth);
+        expect(la.numUnpackedDimensions, clone.numUnpackedDimensions);
+        expect(la.width, clone.width);
+        expect(la.elements.length, clone.elements.length);
+        for (var i = 0; i < la.elements.length; i++) {
+          expect(la.elements[i].width, clone.elements[i].width);
+        }
+        expect(la.name, clone.name);
+        expect(la.isNet, clone.isNet);
+        expect(clone.elements[0].elements[1].isNet, isNet);
+        expect(
+            clone.elements[1].elements[1].elements[1] is LogicArray, isFalse);
+        expect(clone.elements[1].elements[1].elements[1].isNet, isNet);
+      });
+    }
+  });
 }
diff --git a/test/logic_name_test.dart b/test/logic_name_test.dart
index 969614f09..624a6b4d2 100644
--- a/test/logic_name_test.dart
+++ b/test/logic_name_test.dart
@@ -7,9 +7,23 @@
 // 2022 October 26
 // Author: Yao Jing Quek <yao.jing.quek@intel.com>
 
+import 'package:collection/collection.dart';
 import 'package:rohd/rohd.dart';
 import 'package:rohd/src/utilities/sanitizer.dart';
 import 'package:test/test.dart';
+import 'logic_structure_test.dart' as logic_structure_test;
+
+class MyStruct extends LogicStructure {
+  final Logic ready;
+  final Logic valid;
+
+  factory MyStruct() => MyStruct._(
+        Logic(name: 'ready'),
+        Logic(name: 'valid'),
+      );
+
+  MyStruct._(this.ready, this.valid) : super([ready, valid], name: 'myStruct');
+}
 
 class LogicTestModule extends Module {
   LogicTestModule(String logicName) {
@@ -211,4 +225,192 @@ void main() {
             '.o(o),'
             '.portB_1(portB_1),.portB(portB))'));
   });
+
+  group('clone', () {
+    test('name selection', () {
+      const originalName = 'original';
+      for (final newName in ['new', null]) {
+        for (final originalNaming in Naming.values) {
+          for (final newNaming in [...Naming.values, null]) {
+            final selectedName = newName ?? originalName;
+            final selectedNaming = Naming.chooseCloneNaming(
+              originalName: originalName,
+              newName: newName,
+              originalNaming: originalNaming,
+              newNaming: newNaming,
+            );
+
+            final reason = 'original: ($originalName, ${originalNaming.name}),'
+                ' new: ($newName, ${newNaming?.name})'
+                ' => ($selectedName, ${selectedNaming.name})';
+
+            if (newNaming != null) {
+              expect(selectedNaming, newNaming, reason: reason);
+            } else if (newName == null && newNaming == null) {
+              expect(selectedNaming, Naming.mergeable, reason: reason);
+            } else if (newName != null && newNaming == null) {
+              expect(selectedNaming, Naming.renameable, reason: reason);
+            } else {
+              fail('Undefined scenario: $reason');
+            }
+          }
+        }
+      }
+    });
+
+    group('logic', () {
+      test('name null', () {
+        final c = Logic(name: 'a').clone();
+        expect(c.name, 'a');
+        expect(c.naming, Naming.mergeable);
+      });
+
+      test('name provided', () {
+        final c = Logic(name: 'a').clone(name: 'b');
+        expect(c.name, 'b');
+        expect(c.naming, Naming.renameable);
+      });
+
+      test('net', () {
+        final c = LogicNet(name: 'a').clone();
+        expect(c.name, 'a');
+        expect(c.naming, Naming.mergeable);
+        expect(c, isA<LogicNet>());
+      });
+    });
+
+    group('logic structure', () {
+      test('name null', () {
+        final c = MyStruct().clone();
+        expect(c.name, 'myStruct');
+        expect(c.naming, Naming.unnamed);
+      });
+
+      test('name provided', () {
+        final c = MyStruct().clone(name: 'newName');
+        expect(c.name, 'newName');
+        expect(c.naming, Naming.unnamed);
+      });
+    });
+
+    group('logic array', () {
+      test('name null', () {
+        final c = LogicArray([1, 2], 3, name: 'a').clone();
+        expect(c.name, 'a');
+        expect(c.naming, Naming.mergeable);
+      });
+
+      test('name provided', () {
+        final c = LogicArray([1, 2], 3, name: 'a').clone(name: 'b');
+        expect(c.name, 'b');
+        expect(c.naming, Naming.renameable);
+      });
+
+      test('net', () {
+        final c = LogicArray.net([1, 2], 3, name: 'a').clone();
+        expect(c.name, 'a');
+        expect(c.naming, Naming.mergeable);
+        expect(c.isNet, true);
+      });
+    });
+  });
+
+  group('named', () {
+    test('logic', () {
+      final a = Logic(name: 'a');
+      final b = a.named('b');
+
+      a.put(1);
+
+      expect(b.value.toInt(), 1);
+      expect(b.name, 'b');
+      expect(b.naming, Naming.renameable);
+    });
+
+    test('logic with naming', () {
+      final a = Logic(name: 'a');
+      final b = a.named('b', naming: Naming.reserved);
+
+      a.put(1);
+
+      expect(b.value.toInt(), 1);
+      expect(b.name, 'b');
+      expect(b.naming, Naming.reserved);
+    });
+
+    test('net', () {
+      final a = LogicNet(name: 'a');
+      final b = a.named('b');
+
+      a.put(1);
+
+      expect(b.value.toInt(), 1);
+      expect(b.name, 'b');
+      expect(b.naming, Naming.renameable);
+      expect(b.isNet, true);
+      expect(b, isA<LogicNet>());
+    });
+
+    test('array', () {
+      final a = LogicArray([1, 2], 3, name: 'a', numUnpackedDimensions: 1);
+      final b = a.named('b');
+
+      a.elements[0].elements[0].put(1);
+
+      final listEq = const ListEquality<int>().equals;
+
+      expect(b.elements[0].elements[0].value.toInt(), 1);
+      expect(listEq(b.dimensions, a.dimensions), true);
+      expect(b.numUnpackedDimensions, a.numUnpackedDimensions);
+      expect(b.name, 'b');
+      expect(b.naming, Naming.renameable);
+    });
+
+    test('array net with naming', () {
+      final a = LogicArray.net([1, 2], 3, name: 'a', numUnpackedDimensions: 1);
+      final b = a.named('b', naming: Naming.reserved);
+
+      a.elements[0].elements[0].put(1);
+
+      final listEq = const ListEquality<int>().equals;
+
+      expect(b.elements[0].elements[0].value.toInt(), 1);
+      expect(listEq(b.dimensions, a.dimensions), true);
+      expect(b.numUnpackedDimensions, a.numUnpackedDimensions);
+      expect(b.name, 'b');
+      expect(b.naming, Naming.reserved);
+      expect(b.isNet, true);
+      expect(b, isA<LogicArray>());
+    });
+
+    test('structure', () {
+      final a = logic_structure_test.MyFancyStruct();
+      final b = a.named(
+        'b',
+
+        // naming should have no effect
+        naming: Naming.reserved,
+      );
+
+      expect(b.name, 'b');
+
+      expect(b.width, a.width);
+      expect(b.elements[0], isA<LogicArray>());
+      expect(b.elements[0].name, a.elements[0].name);
+      expect(b.elements[0].naming, Naming.renameable);
+
+      expect(b.elements[1], isA<Logic>());
+      expect(b.elements[1].name, a.elements[1].name);
+      expect(b.elements[1].naming, Naming.renameable);
+
+      expect(b.elements[2], isA<logic_structure_test.MyStruct>());
+      expect(b.elements[2].name, a.elements[2].name);
+      expect(b.elements[2].naming, a.elements[2].naming);
+      expect(b.elements[2].elements[0].name, a.elements[2].elements[0].name);
+
+      a.arr.elements[0].put(1);
+
+      expect(b.elements[0].elements[0].value.toInt(), 1);
+    });
+  });
 }
diff --git a/test/logic_structure_test.dart b/test/logic_structure_test.dart
index aa8724e27..35ee34ecd 100644
--- a/test/logic_structure_test.dart
+++ b/test/logic_structure_test.dart
@@ -182,9 +182,23 @@ void main() {
       final copy = orig.clone();
 
       expect(copy.name, orig.name);
+
       expect(copy.width, orig.width);
       expect(copy.elements[0], isA<LogicArray>());
+      expect(copy.elements[0].name, orig.elements[0].name);
+      expect(copy.elements[0].naming, Naming.renameable);
+
+      expect(copy.elements[1], isA<Logic>());
+      expect(copy.elements[1].name, orig.elements[1].name);
+      expect(copy.elements[1].naming, Naming.renameable);
+
       expect(copy.elements[2], isA<MyStruct>());
+      expect(copy.elements[2].name, orig.elements[2].name);
+      expect(copy.elements[2].naming, orig.elements[2].naming);
+      expect(
+          copy.elements[2].elements[0].name, orig.elements[2].elements[0].name);
+
+      expect(orig.clone(name: 'newName').name, 'newName');
     });
   });