Update src and examples to use override keyword #324

voertler · 2025-03-05T05:53:18Z

This PR implemts #289 for src, examples and tests

voertler · 2025-03-05T08:46:01Z

@voertler This needs to be rebased on the current master

during clang-tidy checks.

with virtual/override definitions

where appropriate

-Woverloaded-virtual - synchronize examples with tests/uvm-systemc/examples

voertler · 2025-03-10T05:39:08Z

@barnasc PR has been rebased so that it can be merged

barnasc · 2025-03-10T16:14:15Z

I reviewed the PR and have some questions/remarks:

It seems override is also used for member functions examples where a component is actually considered the only (and final) implementation, so I would expect 'final in such case and not override.
There seems some controversy about adding override to destructors. What is our strategy here?
some nullptr arguments are unintentially replaced by NULL?
in some cases we remove the virtual keyword and now the indentation of arguments is not aligned anymore
some other comments in-line

barnasc

see comments above

tests/tests/uvm-systemc/examples/simple/phases/timeout/tb_env.h

barnasc · 2025-03-10T15:48:35Z

tests/tests/uvm-systemc/examples/integrated/ubus/vip/ubus_transfer.h

+  void do_pack(uvm::uvm_packer& p) const override;
+  void do_unpack(uvm::uvm_packer& p) override;
+  void do_copy(const uvm::uvm_object& rhs) override;
+  bool do_compare(const uvm::uvm_object& rhs, const uvm::uvm_comparer* comparer = nullptr ) const override;


I see we changed the signature of do_compare, where a second argument has been added to specify the comparer. Is this really required? Can we specify the comparer not in a different way?

barnasc · 2025-03-10T15:54:42Z

tests/tests/uvm-systemc/examples/simple/registers/models/aliasing/regmodel.h

                             uvm::uvm_reg_data_t  previous,
-                             uvm::uvm_reg_data_t& value,
+                             uvm::uvm_reg_data_t  value,


We need to check this, I would expect value can change, in this method (in UVM-SV this is an inout type, so we might need to keep it as pass-by-reference. We also need to check the LRM

This is discussed #331

This should be a reference not a copy

tests/tests/uvm-systemc/examples_uvmsc/packer/operators/packet.h

barnasc · 2025-03-10T16:01:37Z

tests/tests/uvm-systemc/unit_tests/00basic/00hello/test.cpp

@@ -32,7 +32,7 @@ class test : public uvm_test
  test( uvm_component_name name ) : uvm_test(name)
  {}

-  virtual void report_phase( uvm_phase& phase )
+   void report_phase( uvm_phase& phase ) override


remove empty space at front

examples/uvmsc/simple/configuration/manual/sc_main.cpp

barnasc · 2025-03-10T16:09:14Z

src/uvmsc/factory/uvm_component_registry.h

@@ -25,11 +25,12 @@

 #ifndef UVM_COMPONENT_REGISTRY_H_
 #define UVM_COMPONENT_REGISTRY_H_
-
+    


remove whitespaces

barnasc · 2025-03-10T16:10:13Z

src/uvmsc/misc/uvm_copy_map.h

@@ -21,6 +21,8 @@
 #define UVM_COPY_MAP_H_

 //////////////
+#include <map>
+#include "uvmsc/base/uvm_object.h"


do we need an include here or is a forward declaration not sufficient?

barnasc · 2025-03-10T16:11:36Z

src/uvmsc/misc/uvm_misc.h

-#include "uvmsc/misc/uvm_status_container.h"
-#include "uvmsc/misc/uvm_copy_map.h"
+
+#include "uvmsc/base/uvm_object.h"


do we need an include here or is a forward declaration enough?

barnasc · 2025-03-10T16:12:54Z

src/uvmsc/reg/uvm_reg_field.h

+  uvm_object* clone() override;
+  void do_copy( const uvm_object& rhs ) override;
+  bool do_compare( const uvm_object& rhs,
+                           const uvm_comparer* comparer ) const override;


fix indentation

voertler · 2025-03-19T08:44:20Z

VWG:

we use override instead of final for the examples and do finetuning for specific examples later
virtual destructors should be marked override as discussed here Closes #1446 isocpp/CppCoreGuidelines#1448

- Fix ;;

voertler added 5 commits March 10, 2025 13:34

Fix header includes to include required headers only, This occurred

d54a8e4

during clang-tidy checks.

Remove non standard function kind() from implementation as it clashes

f33000e

with virtual/override definitions

Add override where appropriate to fix -Winconsistent-missing-override

cebf648

where appropriate

Fix regression tests to avoid -Winconsistent-overrides

0359e97

-Woverloaded-virtual - synchronize examples with tests/uvm-systemc/examples

Fix Merge errors

23dd9ae

voertler force-pushed the fix-override branch from 2cb4b6b to 23dd9ae Compare March 10, 2025 05:33

voertler requested a review from barnasc March 10, 2025 05:38

barnasc requested changes Mar 10, 2025

View reviewed changes

voertler mentioned this pull request Mar 19, 2025

Check whether regmodel uvm::uvm_reg_data_t should be passed as reference #331

Open

voertler added 3 commits April 2, 2025 13:40

Merge branch 'OSCI-WG:master' into fix-override

32c4a41

- Fix NULL

ab40a76

- Fix ;;

Fix copyright year

e82140f

		@@ -25,11 +25,12 @@

		#ifndef UVM_COMPONENT_REGISTRY_H_
		#define UVM_COMPONENT_REGISTRY_H_

Update src and examples to use override keyword #324

Are you sure you want to change the base?

Update src and examples to use override keyword #324

Uh oh!

Conversation

voertler commented Mar 5, 2025

Uh oh!

voertler commented Mar 5, 2025

Uh oh!

voertler commented Mar 10, 2025

Uh oh!

barnasc commented Mar 10, 2025

Uh oh!

barnasc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

voertler commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

barnasc left a comment •

edited

Loading

voertler commented Mar 19, 2025 •

edited

Loading