From e4267e53dac36206f2f737e9745508ca7c201e65 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 25 May 2024 14:23:16 +0200 Subject: [PATCH 01/13] Removed some debugging code --- src/db/unit_tests/dbLayoutQueryTests.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/db/unit_tests/dbLayoutQueryTests.cc b/src/db/unit_tests/dbLayoutQueryTests.cc index b8b6d4c86..85042b5da 100644 --- a/src/db/unit_tests/dbLayoutQueryTests.cc +++ b/src/db/unit_tests/dbLayoutQueryTests.cc @@ -1020,7 +1020,6 @@ void init_layout2 (db::Layout &g) tl::InputStream stream (tl::testdata () + "/gds/issue-1671.gds"); db::Reader reader (stream); reader.read (g, db::LoadLayoutOptions ()); - return; // @@@ g.insert_layer (0, db::LayerProperties ("l0")); g.insert_layer (1, db::LayerProperties ("l1")); From 3ca99907de99242c55ea4ce00558ccd71d6a6589 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 25 May 2024 14:24:00 +0200 Subject: [PATCH 02/13] Improved OASIS-to-OASIS file size by re-introducing sorting of repetition arrays --- .../oasis/db_plugin/dbOASISWriter.cc | 62 ++++++++++--------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/src/plugins/streamers/oasis/db_plugin/dbOASISWriter.cc b/src/plugins/streamers/oasis/db_plugin/dbOASISWriter.cc index fa93a540d..3b6393e38 100644 --- a/src/plugins/streamers/oasis/db_plugin/dbOASISWriter.cc +++ b/src/plugins/streamers/oasis/db_plugin/dbOASISWriter.cc @@ -47,6 +47,36 @@ static const char *s_bounding_box_name = "S_BOUNDING_BOX"; // --------------------------------------------------------------------------------- +/** + * @brief Compare operator for points, distinct x clustered (with same y) + */ +struct vector_cmp_x +{ + bool operator() (const db::Vector &a, const db::Vector &b) const + { + if (a.y () != b.y ()) { + return a.y () < b.y (); + } else { + return a.x () < b.x (); + } + } +}; + +/** + * @brief Compare operator for points, distinct y clustered (with same x) + */ +struct vector_cmp_y +{ + bool operator() (const db::Vector &a, const db::Vector &b) const + { + if (a.x () != b.x ()) { + return a.x () < b.x (); + } else { + return a.y () < b.y (); + } + } +}; + /** * @brief Determines whether a property shall be produced as S_GDS_PROPERTY */ @@ -224,6 +254,7 @@ template void create_repetition_by_type (const db::Shape &array_shap *pw++ = *p - po; } pts.erase (pw, pts.end ()); + std::sort (pts.begin (), pts.end (), vector_cmp_x ()); db::IrregularRepetition *rep_base = new db::IrregularRepetition (); rep_base->points ().swap (pts); @@ -269,36 +300,6 @@ void create_repetition (const db::Shape &array, db::Repetition &rep) } } -/** - * @brief Compare operator for points, distinct x clustered (with same y) - */ -struct vector_cmp_x -{ - bool operator() (const db::Vector &a, const db::Vector &b) const - { - if (a.y () != b.y ()) { - return a.y () < b.y (); - } else { - return a.x () < b.x (); - } - } -}; - -/** - * @brief Compare operator for points, distinct y clustered (with same x) - */ -struct vector_cmp_y -{ - bool operator() (const db::Vector &a, const db::Vector &b) const - { - if (a.x () != b.x ()) { - return a.x () < b.x (); - } else { - return a.y () < b.y (); - } - } -}; - /** * @brief Compare operator for points/abstract repetition pair with configurable point compare operator */ @@ -2030,6 +2031,7 @@ OASISWriter::write (const db::CellInstArray &inst, db::properties_id_type prop_i *pw++ = *p - po; } pts.erase (pw, pts.end ()); + std::sort (pts.begin (), pts.end (), vector_cmp_x ()); db::IrregularRepetition *rep_base = new db::IrregularRepetition (); rep_base->points ().swap (pts); From 370abf7ab3673d243ba57b744c7e7275e7940ef7 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 25 May 2024 17:46:29 +0200 Subject: [PATCH 03/13] Enum to variant binding: needs to use enum class, not EnumWrapper The effect of this fix is that tl::Variant gets enabled for enums declared with gsi::Enum. --- src/gsi/gsi/gsiClass.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/gsi/gsi/gsiClass.h b/src/gsi/gsi/gsiClass.h index a1cf18587..ec29c7ecf 100644 --- a/src/gsi/gsi/gsiClass.h +++ b/src/gsi/gsi/gsiClass.h @@ -445,7 +445,9 @@ struct NoAdaptorTag { }; template struct adaptor_type_info { - static const std::type_info *type_info () + typedef Adapted final_type; + + static const std::type_info *type_info () { return &typeid (Adapted); } @@ -476,6 +478,8 @@ struct adaptor_type_info template struct adaptor_type_info { + typedef X final_type; + static const std::type_info *type_info () { return 0; @@ -517,6 +521,8 @@ class GSI_PUBLIC_TEMPLATE Class : public ClassBase { public: + typedef typename adaptor_type_info::final_type final_type; + Class (const std::string &module, const std::string &name, const Methods &mm, const std::string &doc = std::string (), bool do_register = true) : ClassBase (doc, mm, do_register) { @@ -681,9 +687,9 @@ class GSI_PUBLIC_TEMPLATE Class } private: - gsi::VariantUserClass m_var_cls; - gsi::VariantUserClass m_var_cls_c; - gsi::VariantUserClass m_var_cls_cls; + gsi::VariantUserClass m_var_cls; + gsi::VariantUserClass m_var_cls_c; + gsi::VariantUserClass m_var_cls_cls; std::unique_ptr m_subclass_tester; }; From 1861abc68c80286dfb210bc9aee36d13ad42d29e Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 25 May 2024 17:48:17 +0200 Subject: [PATCH 04/13] Bugfix for passing default arguments to GSI calls This happens when default arguments (specifically user class or enum types) and passed between keyword and positional arguments. We must not use a temporary tl::Variant object as it gets out of scope and a reference is stored. In addition: better error messages for Python when a method can't be matched to arguments. --- src/gsi/gsi/gsiExpression.cc | 6 +++-- src/gsi/gsi_test/gsiTest.cc | 10 +++++-- src/pya/pya/pyaCallables.cc | 43 +++++++++++++++++++++++------- src/rba/rba/rba.cc | 6 +++-- testdata/python/basic.py | 42 +++++++++++++++++++++++++++++ testdata/ruby/basic_testcore.rb | 47 +++++++++++++++++++++++++++++++++ 6 files changed, 139 insertions(+), 15 deletions(-) diff --git a/src/gsi/gsi/gsiExpression.cc b/src/gsi/gsi/gsiExpression.cc index 3c7da5694..af0e9ac6e 100644 --- a/src/gsi/gsi/gsiExpression.cc +++ b/src/gsi/gsi/gsiExpression.cc @@ -980,8 +980,10 @@ VariantUserClassImpl::execute_gsi (const tl::ExpressionParserContext & /*context // leave it to the consumer to establish the default values (that is faster) break; } - tl::Variant def_value = a->spec ()->default_value (); - gsi::push_arg (arglist, *a, def_value, &heap); + const tl::Variant &def_value = a->spec ()->default_value (); + // NOTE: this const_cast means we need to take care that we do not use default values on "out" parameters. + // Otherwise there is a chance we will modify the default value. + gsi::push_arg (arglist, *a, const_cast (def_value), &heap); } else { throw tl::Exception (tl::to_string ("No argument provided (positional or keyword) and no default value available")); } diff --git a/src/gsi/gsi_test/gsiTest.cc b/src/gsi/gsi_test/gsiTest.cc index ee75058a1..04b5dbaca 100644 --- a/src/gsi/gsi_test/gsiTest.cc +++ b/src/gsi/gsi_test/gsiTest.cc @@ -1715,9 +1715,15 @@ gsi::EnumIn enum_in_b3 ("", "E", gsi::enum_const ("E3C", B3::E3C) ); -// 3 base classes +static std::string d4 (BB *, int a, std::string b, double c, B3::E d, tl::Variant e) +{ + return tl::sprintf ("%d,%s,%.12g,%d,%s", a, b, c, int (d), e.to_string ()); +} + +// 3 base classes and enums static gsi::Class decl_bb (decl_b1, "", "BB", - gsi::method ("d3", &BB::d3) + gsi::method ("d3", &BB::d3) + + gsi::method_ext ("d4", &d4, gsi::arg ("a"), gsi::arg ("b"), gsi::arg ("c"), gsi::arg ("d", B3::E3A, "E3A"), gsi::arg ("e", tl::Variant (), "nil"), "") ); gsi::ClassExt b2_in_bb (decl_b2); gsi::ClassExt b3_in_bb (decl_b3); diff --git a/src/pya/pya/pyaCallables.cc b/src/pya/pya/pyaCallables.cc index 77f3c1728..043bd95a2 100644 --- a/src/pya/pya/pyaCallables.cc +++ b/src/pya/pya/pyaCallables.cc @@ -194,18 +194,29 @@ num_args (const gsi::MethodBase *m) } static bool -compatible_with_args (const gsi::MethodBase *m, int argc, PyObject *kwargs) +compatible_with_args (const gsi::MethodBase *m, int argc, PyObject *kwargs, std::string *why_not = 0) { int nargs = num_args (m); - if (argc >= nargs) { + if (argc > nargs) { + if (why_not) { + *why_not = tl::sprintf (tl::to_string (tr ("%d argument(s) expected, but %d given")), nargs, argc); + } + return false; + } else if (argc == nargs) { // no more arguments to consider - return argc == nargs && (kwargs == NULL || PyDict_Size (kwargs) == 0); + if (kwargs != NULL && PyDict_Size (kwargs) > 0) { + if (why_not) { + *why_not = tl::to_string (tr ("all arguments given, but additional keyword arguments specified")); + } + return false; + } else { + return true; + } } if (kwargs != NULL) { - int nkwargs = int (PyDict_Size (kwargs)); int kwargs_taken = 0; while (argc < nargs) { @@ -213,6 +224,9 @@ compatible_with_args (const gsi::MethodBase *m, int argc, PyObject *kwargs) pya::PythonPtr py_arg = PyDict_GetItemString (kwargs, atype.spec ()->name ().c_str ()); if (! py_arg) { if (! atype.spec ()->has_default ()) { + if (why_not) { + *why_not = tl::sprintf (tl::to_string (tr ("no argument specified for '%s' (neither positional or keyword)")), atype.spec ()->name ()); + } return false; } } else { @@ -221,14 +235,20 @@ compatible_with_args (const gsi::MethodBase *m, int argc, PyObject *kwargs) ++argc; } - // matches if all keyword arguments are taken - return kwargs_taken == nkwargs; + return true; } else { while (argc < nargs) { const gsi::ArgType &atype = m->begin_arguments () [argc]; if (! atype.spec ()->has_default ()) { + if (why_not) { + if (argc < nargs - 1 && ! m->begin_arguments () [argc + 1].spec ()->has_default ()) { + *why_not = tl::sprintf (tl::to_string (tr ("no value given for argument #%d and following")), argc + 1); + } else { + *why_not = tl::sprintf (tl::to_string (tr ("no value given for argument #%d")), argc + 1); + } + } return false; } ++argc; @@ -243,8 +263,11 @@ static std::string describe_overload (const gsi::MethodBase *m, int argc, PyObject *kwargs) { std::string res = m->to_string (); - if (compatible_with_args (m, argc, kwargs)) { + std::string why_not; + if (compatible_with_args (m, argc, kwargs, &why_not)) { res += " " + tl::to_string (tr ("[match candidate]")); + } else if (! why_not.empty ()) { + res += " [" + why_not + "]"; } return res; } @@ -705,8 +728,10 @@ push_args (gsi::SerialArgs &arglist, const gsi::MethodBase *meth, PyObject *args // leave it to the consumer to establish the default values (that is faster) break; } - tl::Variant def_value = a->spec ()->default_value (); - gsi::push_arg (arglist, *a, def_value, &heap); + const tl::Variant &def_value = a->spec ()->default_value (); + // NOTE: this const_cast means we need to take care that we do not use default values on "out" parameters. + // Otherwise there is a chance we will modify the default value. + gsi::push_arg (arglist, *a, const_cast (def_value), &heap); } else { throw tl::Exception (tl::to_string (tr ("No argument provided (positional or keyword) and no default value available"))); } diff --git a/src/rba/rba/rba.cc b/src/rba/rba/rba.cc index 0cef54c62..f05031a2c 100644 --- a/src/rba/rba/rba.cc +++ b/src/rba/rba/rba.cc @@ -1066,8 +1066,10 @@ push_args (gsi::SerialArgs &arglist, const gsi::MethodBase *meth, VALUE *argv, i // leave it to the consumer to establish the default values (that is faster) break; } - tl::Variant def_value = a->spec ()->default_value (); - gsi::push_arg (arglist, *a, def_value, &heap); + const tl::Variant &def_value = a->spec ()->default_value (); + // NOTE: this const_cast means we need to take care that we do not use default values on "out" parameters. + // Otherwise there is a chance we will modify the default value. + gsi::push_arg (arglist, *a, const_cast (def_value), &heap); } else { throw tl::Exception (tl::to_string (tr ("No argument provided (positional or keyword) and no default value available"))); } diff --git a/testdata/python/basic.py b/testdata/python/basic.py index 944989467..c24e2d477 100644 --- a/testdata/python/basic.py +++ b/testdata/python/basic.py @@ -3236,6 +3236,48 @@ def test_91(self): self.assertEqual(pc.x, 3) self.assertEqual(pdc.x, 4) + # keyword arguments, enums and error messages + + def test_92(self): + + bb = pya.BB() + + m = "" + try: + bb.d4() + except Exception as ex: + m = str(ex) + self.assertEqual(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #1 and following]\n in BB.d4") + + m = "" + try: + bb.d4(1, "a") + except Exception as ex: + m = str(ex) + self.assertEqual(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #3]\n in BB.d4") + + m = "" + try: + bb.d4(1, "a", 2.0, xxx=17) + except Exception as ex: + m = str(ex) + self.assertEqual(m, "Unknown keyword parameter: xxx in BB.d4") + + m = "" + try: + bb.d4(a=1, b="a", c=2.0, xxx=17) + except Exception as ex: + m = str(ex) + self.assertEqual(m, "Unknown keyword parameter: xxx in BB.d4") + + self.assertEqual(bb.d4(1, "a", 2.0), "1,a,2,100,nil") + self.assertEqual(bb.d4(1, "a", 2.0, e=42), "1,a,2,100,42") + self.assertEqual(bb.d4(1, "a", c=2.0, e=42), "1,a,2,100,42") + self.assertEqual(bb.d4(c=2.0, a=1, b="a", e=42), "1,a,2,100,42") + self.assertEqual(bb.d4(1, "a", 2.0, d=pya.BB.E.E3B), "1,a,2,101,nil") + self.assertEqual(bb.d4(1, "a", d=pya.BB.E.E3B, c=2.5), "1,a,2.5,101,nil") + self.assertEqual(bb.d4(1, "a", 2.0, pya.BB.E.E3B, 42), "1,a,2,101,42") + # run unit tests if __name__ == '__main__': suite = unittest.TestSuite() diff --git a/testdata/ruby/basic_testcore.rb b/testdata/ruby/basic_testcore.rb index bd042373d..60aeff65d 100644 --- a/testdata/ruby/basic_testcore.rb +++ b/testdata/ruby/basic_testcore.rb @@ -3154,4 +3154,51 @@ def test_80 end + # keyword arguments, enums and error messages + def test_81 + + bb = RBA::BB::new + + m = "" + begin + bb.d4() + rescue => ex + m = ex.to_s + end + assert_equal(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #1 and following]\n in BB.d4") + + m = "" + begin + bb.d4(1, "a") + rescue => ex + m = ex.to_s + end + assert_equal(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #3]\n in BB.d4") + + m = "" + begin + bb.d4(1, "a", 2.0, xxx=17) + rescue => ex + m = ex.to_s + end + assert_equal(m, "Unknown keyword parameter: xxx in BB.d4") + + m = "" + begin + bb.d4(a=1, b="a", c=2.0, xxx=17) + rescue => ex + m = ex.to_s + end + assert_equal(m, "Unknown keyword parameter: xxx in BB.d4") + + assert_equal(bb.d4(1, "a", 2.0), "1,a,2,100,nil") + assert_equal(bb.d4(1, "a", 2.0, e=42), "1,a,2,100,42") + assert_equal(bb.d4(1, "a", c=2.0, e=42), "1,a,2,100,42") + assert_equal(bb.d4(c=2.0, a=1, b="a", e=42), "1,a,2,100,42") + assert_equal(bb.d4(1, "a", 2.0, d=RBA::BB::E::E3B), "1,a,2,101,nil") + assert_equal(bb.d4(1, "a", d=RBA::BB::E::E3B, c=2.5), "1,a,2.5,101,nil") + assert_equal(bb.d4(1, "a", 2.0, RBA::BB::E::E3B, 42), "1,a,2,101,42") + + end + end From eb92e5f2d1f000577d1cbfde49a1aa9b7bcd685a Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 25 May 2024 18:43:44 +0200 Subject: [PATCH 05/13] Better error messages on argument mismatch for Ruby too. --- src/rba/rba/rba.cc | 39 ++++++++++++++++++++++++++------- testdata/ruby/basic_testcore.rb | 22 +++++++++---------- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/src/rba/rba/rba.cc b/src/rba/rba/rba.cc index f05031a2c..dee16050b 100644 --- a/src/rba/rba/rba.cc +++ b/src/rba/rba/rba.cc @@ -326,18 +326,29 @@ class MethodTableEntry private: static bool - compatible_with_args (const gsi::MethodBase *m, int argc, VALUE kwargs) + compatible_with_args (const gsi::MethodBase *m, int argc, VALUE kwargs, std::string *why_not = 0) { int nargs = num_args (m); - if (argc >= nargs) { + if (argc > nargs) { + if (why_not) { + *why_not = tl::sprintf (tl::to_string (tr ("%d argument(s) expected, but %d given")), nargs, argc); + } + return false; + } else if (argc == nargs) { // no more arguments to consider - return argc == nargs && (kwargs == Qnil || RHASH_SIZE (kwargs) == 0); + if (kwargs != Qnil && RHASH_SIZE (kwargs) > 0) { + if (why_not) { + *why_not = tl::to_string (tr ("all arguments given, but additional keyword arguments specified")); + } + return false; + } else { + return true; + } } if (kwargs != Qnil) { - int nkwargs = int (RHASH_SIZE (kwargs)); int kwargs_taken = 0; while (argc < nargs) { @@ -345,6 +356,9 @@ class MethodTableEntry VALUE rb_arg = rb_hash_lookup2 (kwargs, ID2SYM (rb_intern (atype.spec ()->name ().c_str ())), Qnil); if (rb_arg == Qnil) { if (! atype.spec ()->has_default ()) { + if (why_not) { + *why_not = tl::sprintf (tl::to_string (tr ("no argument specified for '%s' (neither positional or keyword)")), atype.spec ()->name ()); + } return false; } } else { @@ -353,14 +367,20 @@ class MethodTableEntry ++argc; } - // matches if all keyword arguments are taken - return kwargs_taken == nkwargs; + return true; } else { while (argc < nargs) { const gsi::ArgType &atype = m->begin_arguments () [argc]; if (! atype.spec ()->has_default ()) { + if (why_not) { + if (argc < nargs - 1 && ! m->begin_arguments () [argc + 1].spec ()->has_default ()) { + *why_not = tl::sprintf (tl::to_string (tr ("no value given for argument #%d and following")), argc + 1); + } else { + *why_not = tl::sprintf (tl::to_string (tr ("no value given for argument #%d")), argc + 1); + } + } return false; } ++argc; @@ -375,8 +395,11 @@ class MethodTableEntry describe_overload (const gsi::MethodBase *m, int argc, VALUE kwargs) { std::string res = m->to_string (); - if (compatible_with_args (m, argc, kwargs)) { + std::string why_not; + if (compatible_with_args (m, argc, kwargs, &why_not)) { res += " " + tl::to_string (tr ("[match candidate]")); + } else if (! why_not.empty ()) { + res += " [" + why_not + "]"; } return res; } @@ -1043,7 +1066,7 @@ static gsi::ArgType s_void_type = create_void_type (); static int get_kwargs_keys (VALUE key, VALUE, VALUE arg) { std::set *names = reinterpret_cast *> (arg); - names->insert (ruby2c (key)); + names->insert (ruby2c (rba_safe_obj_as_string (key))); return ST_CONTINUE; } diff --git a/testdata/ruby/basic_testcore.rb b/testdata/ruby/basic_testcore.rb index 60aeff65d..4464012af 100644 --- a/testdata/ruby/basic_testcore.rb +++ b/testdata/ruby/basic_testcore.rb @@ -3165,7 +3165,7 @@ def test_81 rescue => ex m = ex.to_s end - assert_equal(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #1 and following]\n in BB.d4") + assert_equal(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #1 and following]\n in BB::d4") m = "" begin @@ -3173,30 +3173,30 @@ def test_81 rescue => ex m = ex.to_s end - assert_equal(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #3]\n in BB.d4") + assert_equal(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #3]\n in BB::d4") m = "" begin - bb.d4(1, "a", 2.0, xxx=17) + bb.d4(1, "a", 2.0, xxx: 17) rescue => ex m = ex.to_s end - assert_equal(m, "Unknown keyword parameter: xxx in BB.d4") + assert_equal(m, "Unknown keyword parameter: xxx in BB::d4") m = "" begin - bb.d4(a=1, b="a", c=2.0, xxx=17) + bb.d4(a: 1, b: "a", c: 2.0, xxx: 17) rescue => ex m = ex.to_s end - assert_equal(m, "Unknown keyword parameter: xxx in BB.d4") + assert_equal(m, "Unknown keyword parameter: xxx in BB::d4") assert_equal(bb.d4(1, "a", 2.0), "1,a,2,100,nil") - assert_equal(bb.d4(1, "a", 2.0, e=42), "1,a,2,100,42") - assert_equal(bb.d4(1, "a", c=2.0, e=42), "1,a,2,100,42") - assert_equal(bb.d4(c=2.0, a=1, b="a", e=42), "1,a,2,100,42") - assert_equal(bb.d4(1, "a", 2.0, d=RBA::BB::E::E3B), "1,a,2,101,nil") - assert_equal(bb.d4(1, "a", d=RBA::BB::E::E3B, c=2.5), "1,a,2.5,101,nil") + assert_equal(bb.d4(1, "a", 2.0, e: 42), "1,a,2,100,42") + assert_equal(bb.d4(1, "a", c=2.0, e: 42), "1,a,2,100,42") + assert_equal(bb.d4(c: 2.0, a: 1, b: "a", e: 42), "1,a,2,100,42") + assert_equal(bb.d4(1, "a", 2.0, d: RBA::BB::E::E3B), "1,a,2,101,nil") + assert_equal(bb.d4(1, "a", d: RBA::BB::E::E3B, c: 2.5), "1,a,2.5,101,nil") assert_equal(bb.d4(1, "a", 2.0, RBA::BB::E::E3B, 42), "1,a,2,101,42") end From d59d31821860bdbcdcfa2e38fb2a31883a537d20 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 25 May 2024 18:58:18 +0200 Subject: [PATCH 06/13] Bugfix: restoring original overload matching scheme for Ruby and Python --- src/pya/pya/pyaCallables.cc | 58 ++++++++++++++++++++++----------- src/rba/rba/rba.cc | 48 +++++++++++++++++++++------ testdata/python/basic.py | 4 +-- testdata/ruby/basic_testcore.rb | 4 +-- 4 files changed, 81 insertions(+), 33 deletions(-) diff --git a/src/pya/pya/pyaCallables.cc b/src/pya/pya/pyaCallables.cc index 043bd95a2..05b463f40 100644 --- a/src/pya/pya/pyaCallables.cc +++ b/src/pya/pya/pyaCallables.cc @@ -193,10 +193,33 @@ num_args (const gsi::MethodBase *m) return int (m->end_arguments () - m->begin_arguments ()); } +std::set +invalid_kwnames (const gsi::MethodBase *meth, PyObject *kwargs) +{ + pya::PythonRef keys (PyDict_Keys (kwargs)); + + std::set valid_names; + for (gsi::MethodBase::argument_iterator a = meth->begin_arguments (); a != meth->end_arguments (); ++a) { + valid_names.insert (a->spec ()->name ()); + } + + std::set invalid_names; + for (int i = int (PyList_Size (keys.get ())); i > 0; ) { + --i; + std::string k = python2c (PyList_GetItem (keys.get (), i)); + if (valid_names.find (k) == valid_names.end ()) { + invalid_names.insert (k); + } + } + + return invalid_names; +} + static bool compatible_with_args (const gsi::MethodBase *m, int argc, PyObject *kwargs, std::string *why_not = 0) { int nargs = num_args (m); + int nkwargs = kwargs == NULL ? 0 : int (PyDict_Size (kwargs)); if (argc > nargs) { if (why_not) { @@ -205,7 +228,7 @@ compatible_with_args (const gsi::MethodBase *m, int argc, PyObject *kwargs, std: return false; } else if (argc == nargs) { // no more arguments to consider - if (kwargs != NULL && PyDict_Size (kwargs) > 0) { + if (nkwargs > 0) { if (why_not) { *why_not = tl::to_string (tr ("all arguments given, but additional keyword arguments specified")); } @@ -235,7 +258,20 @@ compatible_with_args (const gsi::MethodBase *m, int argc, PyObject *kwargs, std: ++argc; } - return true; + if (kwargs_taken != nkwargs) { + if (why_not) { + std::set invalid_names = invalid_kwnames (m, kwargs); + if (invalid_names.size () > 1) { + std::string names_str = tl::join (invalid_names.begin (), invalid_names.end (), ", "); + *why_not = tl::to_string (tr ("unknown keyword parameters: ")) + names_str; + } else if (invalid_names.size () == 1) { + *why_not = tl::to_string (tr ("unknown keyword parameter: ")) + *invalid_names.begin (); + } + } + return false; + } else { + return true; + } } else { @@ -746,23 +782,7 @@ push_args (gsi::SerialArgs &arglist, const gsi::MethodBase *meth, PyObject *args if (kwargs_taken != nkwargs) { // check if there are any left-over keyword parameters with unknown names - - pya::PythonRef keys (PyDict_Keys (kwargs)); - - std::set valid_names; - for (gsi::MethodBase::argument_iterator a = meth->begin_arguments (); a != meth->end_arguments (); ++a) { - valid_names.insert (a->spec ()->name ()); - } - - std::set invalid_names; - for (int i = int (PyList_Size (keys.get ())); i > 0; ) { - --i; - std::string k = python2c (PyList_GetItem (keys.get (), i)); - if (valid_names.find (k) == valid_names.end ()) { - invalid_names.insert (k); - } - } - + std::set invalid_names = invalid_kwnames (meth, kwargs); if (invalid_names.size () > 1) { std::string names_str = tl::join (invalid_names.begin (), invalid_names.end (), ", "); throw tl::Exception (tl::to_string (tr ("Unknown keyword parameters: ")) + names_str); diff --git a/src/rba/rba/rba.cc b/src/rba/rba/rba.cc index dee16050b..e6ab2a357 100644 --- a/src/rba/rba/rba.cc +++ b/src/rba/rba/rba.cc @@ -182,6 +182,27 @@ get_kwarg (const gsi::ArgType &atype, VALUE kwargs) } } +static int get_kwargs_keys (VALUE key, VALUE, VALUE arg) +{ + std::set *names = reinterpret_cast *> (arg); + names->insert (ruby2c (rba_safe_obj_as_string (key))); + + return ST_CONTINUE; +} + +static std::set +invalid_kwnames (const gsi::MethodBase *meth, VALUE kwargs) +{ + std::set invalid_names; + rb_hash_foreach (kwargs, (int (*)(...)) &get_kwargs_keys, (VALUE) &invalid_names); + + for (gsi::MethodBase::argument_iterator a = meth->begin_arguments (); a != meth->end_arguments (); ++a) { + invalid_names.erase (a->spec ()->name ()); + } + + return invalid_names; +} + // ------------------------------------------------------------------- // The lookup table for the method overload resolution @@ -325,10 +346,12 @@ class MethodTableEntry } private: + static bool compatible_with_args (const gsi::MethodBase *m, int argc, VALUE kwargs, std::string *why_not = 0) { int nargs = num_args (m); + int nkwargs = kwargs == Qnil ? 0 : RHASH_SIZE (kwargs); if (argc > nargs) { if (why_not) { @@ -337,7 +360,7 @@ class MethodTableEntry return false; } else if (argc == nargs) { // no more arguments to consider - if (kwargs != Qnil && RHASH_SIZE (kwargs) > 0) { + if (nkwargs > 0) { if (why_not) { *why_not = tl::to_string (tr ("all arguments given, but additional keyword arguments specified")); } @@ -367,7 +390,20 @@ class MethodTableEntry ++argc; } - return true; + if (kwargs_taken != nkwargs) { + if (why_not) { + std::set invalid_names = invalid_kwnames (m, kwargs); + if (invalid_names.size () > 1) { + std::string names_str = tl::join (invalid_names.begin (), invalid_names.end (), ", "); + *why_not = tl::to_string (tr ("unknown keyword parameters: ")) + names_str; + } else if (invalid_names.size () == 1) { + *why_not = tl::to_string (tr ("unknown keyword parameter: ")) + *invalid_names.begin (); + } + } + return false; + } else { + return true; + } } else { @@ -1063,14 +1099,6 @@ static gsi::ArgType create_void_type () static gsi::ArgType s_void_type = create_void_type (); -static int get_kwargs_keys (VALUE key, VALUE, VALUE arg) -{ - std::set *names = reinterpret_cast *> (arg); - names->insert (ruby2c (rba_safe_obj_as_string (key))); - - return ST_CONTINUE; -} - void push_args (gsi::SerialArgs &arglist, const gsi::MethodBase *meth, VALUE *argv, int argc, VALUE kwargs, tl::Heap &heap) { diff --git a/testdata/python/basic.py b/testdata/python/basic.py index c24e2d477..2c43d39cd 100644 --- a/testdata/python/basic.py +++ b/testdata/python/basic.py @@ -3261,14 +3261,14 @@ def test_92(self): bb.d4(1, "a", 2.0, xxx=17) except Exception as ex: m = str(ex) - self.assertEqual(m, "Unknown keyword parameter: xxx in BB.d4") + self.assertEqual(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [unknown keyword parameter: xxx]\n in BB.d4") m = "" try: bb.d4(a=1, b="a", c=2.0, xxx=17) except Exception as ex: m = str(ex) - self.assertEqual(m, "Unknown keyword parameter: xxx in BB.d4") + self.assertEqual(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [unknown keyword parameter: xxx]\n in BB.d4") self.assertEqual(bb.d4(1, "a", 2.0), "1,a,2,100,nil") self.assertEqual(bb.d4(1, "a", 2.0, e=42), "1,a,2,100,42") diff --git a/testdata/ruby/basic_testcore.rb b/testdata/ruby/basic_testcore.rb index 4464012af..b706f9ee7 100644 --- a/testdata/ruby/basic_testcore.rb +++ b/testdata/ruby/basic_testcore.rb @@ -3181,7 +3181,7 @@ def test_81 rescue => ex m = ex.to_s end - assert_equal(m, "Unknown keyword parameter: xxx in BB::d4") + assert_equal(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [unknown keyword parameter: xxx]\n in BB::d4") m = "" begin @@ -3189,7 +3189,7 @@ def test_81 rescue => ex m = ex.to_s end - assert_equal(m, "Unknown keyword parameter: xxx in BB::d4") + assert_equal(m, "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [unknown keyword parameter: xxx]\n in BB::d4") assert_equal(bb.d4(1, "a", 2.0), "1,a,2,100,nil") assert_equal(bb.d4(1, "a", 2.0, e: 42), "1,a,2,100,42") From 4f88ff68da6fe69478bc31b57aa10dca1c51fa26 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 25 May 2024 19:18:15 +0200 Subject: [PATCH 07/13] Better error messages on Expressions too. --- src/gsi/gsi/gsiExpression.cc | 82 ++++++++++++++++++------ src/gsi/unit_tests/gsiExpressionTests.cc | 57 ++++++++++++++++ testdata/ruby/basic_testcore.rb | 2 +- 3 files changed, 121 insertions(+), 20 deletions(-) diff --git a/src/gsi/gsi/gsiExpression.cc b/src/gsi/gsi/gsiExpression.cc index af0e9ac6e..3d77ddada 100644 --- a/src/gsi/gsi/gsiExpression.cc +++ b/src/gsi/gsi/gsiExpression.cc @@ -721,19 +721,49 @@ num_args (const gsi::MethodBase *m) return int (m->end_arguments () - m->begin_arguments ()); } +std::set +invalid_kwnames (const gsi::MethodBase *meth, const std::map *kwargs) +{ + std::set valid_names; + for (gsi::MethodBase::argument_iterator a = meth->begin_arguments (); a != meth->end_arguments (); ++a) { + valid_names.insert (a->spec ()->name ()); + } + + std::set invalid_names; + for (auto i = kwargs->begin (); i != kwargs->end (); ++i) { + if (valid_names.find (i->first) == valid_names.end ()) { + invalid_names.insert (i->first); + } + } + + return invalid_names; +} + static bool -compatible_with_args (const gsi::MethodBase *m, int argc, const std::map *kwargs) +compatible_with_args (const gsi::MethodBase *m, int argc, const std::map *kwargs, std::string *why_not = 0) { int nargs = num_args (m); + int nkwargs = kwargs ? int (kwargs->size ()) : 0; - if (argc >= nargs) { + if (argc > nargs) { + if (why_not) { + *why_not = tl::sprintf (tl::to_string (tr ("%d argument(s) expected, but %d given")), nargs, argc); + } + return false; + } else if (argc == nargs) { // no more arguments to consider - return argc == nargs && (! kwargs || kwargs->empty ()); + if (nkwargs > 0) { + if (why_not) { + *why_not = tl::to_string (tr ("all arguments given, but additional keyword arguments specified")); + } + return false; + } else { + return true; + } } if (kwargs) { - int nkwargs = int (kwargs->size ()); int kwargs_taken = 0; while (argc < nargs) { @@ -741,6 +771,9 @@ compatible_with_args (const gsi::MethodBase *m, int argc, const std::mapfind (atype.spec ()->name ()); if (i == kwargs->end ()) { if (! atype.spec ()->has_default ()) { + if (why_not) { + *why_not = tl::sprintf (tl::to_string (tr ("no argument specified for '%s' (neither positional or keyword)")), atype.spec ()->name ()); + } return false; } } else { @@ -750,13 +783,33 @@ compatible_with_args (const gsi::MethodBase *m, int argc, const std::map invalid_names = invalid_kwnames (m, kwargs); + if (invalid_names.size () > 1) { + std::string names_str = tl::join (invalid_names.begin (), invalid_names.end (), ", "); + *why_not = tl::to_string (tr ("unknown keyword parameters: ")) + names_str; + } else if (invalid_names.size () == 1) { + *why_not = tl::to_string (tr ("unknown keyword parameter: ")) + *invalid_names.begin (); + } + } + return false; + } else { + return true; + } } else { while (argc < nargs) { const gsi::ArgType &atype = m->begin_arguments () [argc]; if (! atype.spec ()->has_default ()) { + if (why_not) { + if (argc < nargs - 1 && ! m->begin_arguments () [argc + 1].spec ()->has_default ()) { + *why_not = tl::sprintf (tl::to_string (tr ("no value given for argument #%d and following")), argc + 1); + } else { + *why_not = tl::sprintf (tl::to_string (tr ("no value given for argument #%d")), argc + 1); + } + } return false; } ++argc; @@ -771,8 +824,11 @@ static std::string describe_overload (const gsi::MethodBase *m, int argc, const std::map *kwargs) { std::string res = m->to_string (); - if (compatible_with_args (m, argc, kwargs)) { + std::string why_not; + if (compatible_with_args (m, argc, kwargs, &why_not)) { res += " " + tl::to_string (tr ("[match candidate]")); + } else if (! why_not.empty ()) { + res += " [" + why_not + "]"; } return res; } @@ -1006,19 +1062,7 @@ VariantUserClassImpl::execute_gsi (const tl::ExpressionParserContext & /*context if (kwargs_taken != nkwargs) { // check if there are any left-over keyword parameters with unknown names - - std::set valid_names; - for (gsi::MethodBase::argument_iterator a = meth->begin_arguments (); a != meth->end_arguments (); ++a) { - valid_names.insert (a->spec ()->name ()); - } - - std::set invalid_names; - for (auto i = kwargs->begin (); i != kwargs->end (); ++i) { - if (valid_names.find (i->first) == valid_names.end ()) { - invalid_names.insert (i->first); - } - } - + std::set invalid_names = invalid_kwnames (meth, kwargs); if (invalid_names.size () > 1) { std::string names_str = tl::join (invalid_names.begin (), invalid_names.end (), ", "); throw tl::Exception (tl::to_string (tr ("Unknown keyword parameters: ")) + names_str); diff --git a/src/gsi/unit_tests/gsiExpressionTests.cc b/src/gsi/unit_tests/gsiExpressionTests.cc index 4aba5c917..2f83ffce0 100644 --- a/src/gsi/unit_tests/gsiExpressionTests.cc +++ b/src/gsi/unit_tests/gsiExpressionTests.cc @@ -772,3 +772,60 @@ TEST(14) EXPECT_EQ (ex.msg ().find ("No overload with matching arguments. Variants are:"), 0); } } + +TEST(15) +{ + // Keyword arguments, enums and errors + + tl::Eval e; + tl::Variant v; + + try { + v = e.parse("var bb = BB.new; bb.d4()").execute(); + EXPECT_EQ (true, false); + } catch (tl::Exception &ex) { + EXPECT_EQ (ex.msg (), "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #1 and following]\n at position 19 (...d4())"); + } + + try { + v = e.parse("var bb = BB.new; bb.d4(1, 'a')").execute(); + EXPECT_EQ (true, false); + } catch (tl::Exception &ex) { + EXPECT_EQ (ex.msg (), "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [no value given for argument #3]\n at position 19 (...d4(1, 'a'))"); + } + + try { + v = e.parse("var bb = BB.new; bb.d4(1, 'a', 2.0, xxx=17)").execute(); + EXPECT_EQ (true, false); + } catch (tl::Exception &ex) { + EXPECT_EQ (ex.msg (), "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [unknown keyword parameter: xxx]\n at position 19 (...d4(1, 'a', 2.0, xxx..)"); + } + + try { + v = e.parse("var bb = BB.new; bb.d4(a=1, b='a', c=2.0, xxx=17)").execute(); + EXPECT_EQ (true, false); + } catch (tl::Exception &ex) { + EXPECT_EQ (ex.msg (), "Can't match arguments. Variants are:\n string d4(int a, string b, double c, B3::E d = E3A, variant e = nil) [unknown keyword parameter: xxx]\n at position 19 (...d4(a=1, b='a', c=2...)"); + } + + v = e.parse("var bb = BB.new; bb.d4(1, 'a', 2.0)").execute(); + EXPECT_EQ (v.to_string (), "1,a,2,100,nil"); + + v = e.parse("var bb = BB.new; bb.d4(1, 'a', 2.0, e=42)").execute(); + EXPECT_EQ (v.to_string (), "1,a,2,100,42"); + + v = e.parse("var bb = BB.new; bb.d4(1, 'a', c=2.0, e=42)").execute(); + EXPECT_EQ (v.to_string (), "1,a,2,100,42"); + + v = e.parse("var bb = BB.new; bb.d4(c=2.0, a=1, b='a', e=42)").execute(); + EXPECT_EQ (v.to_string (), "1,a,2,100,42"); + + v = e.parse("var bb = BB.new; bb.d4(1, 'a', 2.0, d=BB.E.E3B)").execute(); + EXPECT_EQ (v.to_string (), "1,a,2,101,nil"); + + v = e.parse("var bb = BB.new; bb.d4(1, 'a', d=BB.E.E3B, c=2.0)").execute(); + EXPECT_EQ (v.to_string (), "1,a,2,101,nil"); + + v = e.parse("var bb = BB.new; bb.d4(1, 'a', 2.0, BB.E.E3B, 42)").execute(); + EXPECT_EQ (v.to_string (), "1,a,2,101,42"); +} diff --git a/testdata/ruby/basic_testcore.rb b/testdata/ruby/basic_testcore.rb index b706f9ee7..2cbfce073 100644 --- a/testdata/ruby/basic_testcore.rb +++ b/testdata/ruby/basic_testcore.rb @@ -3193,7 +3193,7 @@ def test_81 assert_equal(bb.d4(1, "a", 2.0), "1,a,2,100,nil") assert_equal(bb.d4(1, "a", 2.0, e: 42), "1,a,2,100,42") - assert_equal(bb.d4(1, "a", c=2.0, e: 42), "1,a,2,100,42") + assert_equal(bb.d4(1, "a", c: 2.0, e: 42), "1,a,2,100,42") assert_equal(bb.d4(c: 2.0, a: 1, b: "a", e: 42), "1,a,2,100,42") assert_equal(bb.d4(1, "a", 2.0, d: RBA::BB::E::E3B), "1,a,2,101,nil") assert_equal(bb.d4(1, "a", d: RBA::BB::E::E3B, c: 2.5), "1,a,2.5,101,nil") From 1677111735ddd8485f4ab6c50d52dd06468b791c Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 25 May 2024 19:57:51 +0200 Subject: [PATCH 08/13] Enhanced documentation for LayoutToNetlist class, two more methods (layer_indexes, layer_info) --- src/db/db/gsiDeclDbLayoutToNetlist.cc | 77 +++++++++++++++++++++++++-- testdata/ruby/dbLayoutToNetlist.rb | 2 + 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/src/db/db/gsiDeclDbLayoutToNetlist.cc b/src/db/db/gsiDeclDbLayoutToNetlist.cc index a9c06d59d..e041f5ae9 100644 --- a/src/db/db/gsiDeclDbLayoutToNetlist.cc +++ b/src/db/db/gsiDeclDbLayoutToNetlist.cc @@ -98,6 +98,24 @@ static std::vector l2n_layer_names (const db::LayoutToNetlist *l2n) return ln; } +static std::vector l2n_layer_indexes (const db::LayoutToNetlist *l2n) +{ + std::vector li; + for (db::LayoutToNetlist::layer_iterator l = l2n->begin_layers (); l != l2n->end_layers (); ++l) { + li.push_back (l->first); + } + return li; +} + +static db::LayerProperties l2n_layer_info (const db::LayoutToNetlist *l2n, unsigned int layer) +{ + if (! l2n->internal_layout () || ! l2n->internal_layout ()->is_valid_layer (layer)) { + return db::LayerProperties (); + } else { + return l2n->internal_layout ()->get_properties (layer); + } +} + static db::Region antenna_check3 (db::LayoutToNetlist *l2n, const db::Region &poly, double poly_area_factor, double poly_perimeter_factor, const db::Region &metal, double metal_area_factor, double metal_perimeter_factor, double ratio, const std::vector &diodes, db::Texts *texts) { std::vector > diode_pairs; @@ -331,7 +349,22 @@ Class decl_dbLayoutToNetlist ("db", "LayoutToNetlist", "This method has been generalized in version 0.27.\n" ) + gsi::method_ext ("layer_names", &l2n_layer_names, - "@brief Returns a list of names of the layer kept inside the LayoutToNetlist object." + "@brief Returns a list of names of the layers kept inside the LayoutToNetlist object." + ) + + gsi::method_ext ("layer_indexes", &l2n_layer_indexes, + "@brief Returns a list of indexes of the layers kept inside the LayoutToNetlist object.\n" + "You can use \\layer_name to get the name from a layer index. You can use \\layer_info to get " + "the \\LayerInfo object attached to a layer - if the layer is an original layer.\n" + "\n" + "This method has been introduced in version 0.29.2.\n" + ) + + gsi::method_ext ("layer_info", &l2n_layer_info, gsi::arg ("index"), + "@brief Returns the LayerInfo object attached to a layer (by index).\n" + "If the layer is an original layer and not a derived one, this method will return the " + "stream layer information where the original layer was taken from. Otherwise an empty \\LayerInfo object " + "is returned.\n" + "\n" + "This method has been introduced in version 0.29.2.\n" ) + gsi::factory ("layer_by_name", &db::LayoutToNetlist::layer_by_name, gsi::arg ("name"), "@brief Gets a layer object for the given name.\n" @@ -599,8 +632,10 @@ Class decl_dbLayoutToNetlist ("db", "LayoutToNetlist", ) + gsi::method_ext ("internal_layout", &l2n_internal_layout, "@brief Gets the internal layout\n" - "Usually it should not be required to obtain the internal layout. If you need to do so, make sure not to modify the layout as\n" - "the functionality of the netlist extractor depends on it." + "The internal layout is where the LayoutToNetlist database stores the shapes for the nets. " + "Usually you do not need to access this object - you must use \\build_net or \\shapes_of_net to " + "retrieve the per-net shape information. If you access the internal layout, make sure you do not " + "modify it." ) + gsi::method_ext ("internal_top_cell", &l2n_internal_top_cell, "@brief Gets the internal top cell\n" @@ -664,8 +699,13 @@ Class decl_dbLayoutToNetlist ("db", "LayoutToNetlist", "This method puts the shapes of a net into the given target cell using a variety of options\n" "to represent the net name and the hierarchy of the net.\n" "\n" - "If the netname_prop name is not nil, a property with the given name is created and assigned\n" - "the net name.\n" + "If 'netname_prop' is not nil, a property with the given name is created and attached to shapes. The value " + "of the property is the net name.\n" + "\n" + "'lmap' defines which layers are to be produced. It is map, where the keys are layer indexes in the " + "target layout and the values are Region objects indicating the layer where shapes are to be taken from. " + "Use \\layer_by_name or \\layer_by_index to get the Region object corresponding to a layer stored inside " + "the LayoutToNetlist database.\n" "\n" "Net hierarchy is covered in three ways:\n" "@ul\n" @@ -696,6 +736,14 @@ Class decl_dbLayoutToNetlist ("db", "LayoutToNetlist", "If no mapping is provided for a specific circuit cell, the nets are copied into the next mapped parent as " "many times as the circuit cell appears there (circuit flattening).\n" "\n" + "If 'netname_prop' is not nil, a property with the given name is created and attached to shapes. The value " + "of the property is the net name.\n" + "\n" + "'lmap' defines which layers are to be produced. It is map, where the keys are layer indexes in the " + "target layout and the values are Region objects indicating the layer where shapes are to be taken from. " + "Use \\layer_by_name or \\layer_by_index to get the Region object corresponding to a layer stored inside " + "the LayoutToNetlist database.\n" + "\n" "The method has three net annotation modes:\n" "@ul\n" " @li No annotation (net_cell_name_prefix == nil and netname_prop == nil): the shapes will be put\n" @@ -910,6 +958,25 @@ Class decl_dbLayoutToNetlist ("db", "LayoutToNetlist", "hierarchical data and an existing DeepShapeStore object, use the " "'LayoutToNetlist(dss)' constructor.\n" "\n" + "Once the extraction is done, you can persist the \\LayoutToNetlist object " + "using \\write and restore it using \\read. You can use the query API (see below) to " + "analyze the LayoutToNetlist database.\n" + "\n" + "The query API of the \\LayoutToNetlist object consists of the following parts:\n" + "\n" + "@ul\n" + "@li Net shape retrieval: \\build_all_nets, \\build_nets, \\build_net and \\shapes_per_net @/li\n" + "@li Layers: \\layer_by_index, \\layer_by_name, \\layer_indexes, \\layer_names, \\layer_info, \\layer_name @/li\n" + "@li Log entries: \\each_log_entry @/li\n" + "@li Probing (get net from position): \\probe_net @/li\n" + "@li Netlist: \\netlist @/li\n" + "@li Internal shape storage: \\internal_layout, \\internal_top_cell @/li\n" + "@li Helper functions: \\cell_mapping_into, \\const_cell_mapping_into @/li\n" + "@/ul\n" + "\n" + "The \\LayoutToNetlist object is also the entry point for connectivity-aware DRC checks, " + "such as antenna checks.\n" + "\n" "This class has been introduced in version 0.26." ); diff --git a/testdata/ruby/dbLayoutToNetlist.rb b/testdata/ruby/dbLayoutToNetlist.rb index a4885ac30..44bb782af 100644 --- a/testdata/ruby/dbLayoutToNetlist.rb +++ b/testdata/ruby/dbLayoutToNetlist.rb @@ -551,6 +551,8 @@ def test_13_ReadAndWrite assert_equal(File.open(tmp, "r").read, File.open(input, "r").read) assert_equal(l2n.layer_names.join(","), "poly,poly_lbl,diff_cont,poly_cont,metal1,metal1_lbl,via1,metal2,metal2_lbl,psd,nsd") + assert_equal(l2n.layer_indexes.join(","), "1,2,3,4,5,6,7,8,9,10,11") + assert_equal(l2n.layer_indexes.collect { |li| l2n.layer_info(li).to_s }.join(","), "3/0,3/1,4/0,5/0,6/0,6/1,7/0,8/0,8/1,,") assert_equal(l2n.layer_name(l2n.layer_by_name("metal1")), "metal1") assert_equal(l2n.layer_name(l2n.layer_by_index(l2n.layer_of(l2n.layer_by_name("metal1")))), "metal1") From 149c97217258a87a7de1803fc3dcd27592a94652 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Wed, 29 May 2024 22:36:33 +0200 Subject: [PATCH 09/13] Enhancing 'blend-mode' 0 (buddy tools) such that it will not generate instance duplicates --- src/buddies/src/bd/bdReaderOptions.cc | 7 +++++-- src/db/db/dbCommonReader.cc | 8 +++++++- src/db/db/dbInstances.h | 15 ++++++++++++++- testdata/gds/collect_add_au.gds | Bin 13406 -> 13292 bytes 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/buddies/src/bd/bdReaderOptions.cc b/src/buddies/src/bd/bdReaderOptions.cc index ebeaaf870..fb1d05445 100644 --- a/src/buddies/src/bd/bdReaderOptions.cc +++ b/src/buddies/src/bd/bdReaderOptions.cc @@ -197,10 +197,13 @@ GenericReaderOptions::add_options (tl::CommandLineOptions &cmd) "--" + m_long_prefix + "blend-mode=mode", &m_cell_conflict_resolution, "Specifies how cell conflicts are resolved when using file concatenation", "When concatenating files with '+', the reader will handle cells with identical names according to this mode:\n" "\n" - "* 0: joins everything (unsafe)\n" + "* 0: joins everything (usually unsafe)\n" "* 1: overwrite\n" "* 2: skip new cell\n" - "* 3: rename cell (safe, default)" + "* 3: rename cell (safe, default)\n" + "\n" + "Mode 0 is a safe solution for the 'same hierarchy, different layers' case. Mode 3 is a safe solution for " + "joining multiple files into one and combining the hierarchy tree of all files as distinct separate trees.\n" ) ; } diff --git a/src/db/db/dbCommonReader.cc b/src/db/db/dbCommonReader.cc index dc87204c9..fb4f5dfe7 100644 --- a/src/db/db/dbCommonReader.cc +++ b/src/db/db/dbCommonReader.cc @@ -245,10 +245,16 @@ CommonReaderBase::merge_cell (db::Layout &layout, db::cell_index_type target_cel db::Cell &target_cell = layout.cell (target_cell_index); target_cell.set_ghost_cell (src_cell.is_ghost_cell () && target_cell.is_ghost_cell ()); + // avoid generating duplicates + std::set current; + for (db::Cell::const_iterator i = target_cell.begin (); ! i.at_end (); ++i) { + current.insert (*i); + } + // copy over the instances for (db::Cell::const_iterator i = src_cell.begin (); ! i.at_end (); ++i) { // NOTE: cell indexed may be invalid because we delete subcells without update() - if (layout.is_valid_cell_index (i->cell_index ())) { + if (layout.is_valid_cell_index (i->cell_index ()) && current.find (*i) == current.end ()) { target_cell.insert (*i); } } diff --git a/src/db/db/dbInstances.h b/src/db/db/dbInstances.h index 595a5f7d0..975904e28 100644 --- a/src/db/db/dbInstances.h +++ b/src/db/db/dbInstances.h @@ -1977,7 +1977,20 @@ OverlappingInstanceIteratorTraits::instance_from_stable_iter (const Iter &iter) // box tree iterators deliver pointers, not iterators. Use instance_from_pointer to do this conversion. return mp_insts->instance_from_pointer (&*iter); } - + +/** + * @brief A compare function for db::Instance that uses "less" for value compare + * + * In contrast, "operator<" will compare the instance reference, not value. + */ +struct InstanceCompareFunction +{ + bool operator() (const db::Instance &a, const db::Instance &b) const + { + return a.less (b); + } +}; + } #endif diff --git a/testdata/gds/collect_add_au.gds b/testdata/gds/collect_add_au.gds index ec52fb206812965fa59b229753e871bc520c7aa4..b021521a81c58832235cef4463c2658404635fe8 100644 GIT binary patch delta 954 zcmcbY@g`k~fsKKQDS|GSB3u;*gmjKOKjRjeFkjKE}u>_Y#|g+bw-o1J7u$)u>~UlOz?QGr6faWG2W@$01{*;EzLQfr7piR{t=tv9X0t-pwjAxmJM>hh_~$Tu!;9 zh|6O=N>_21R;K(KmyF6u>@u4}RLdE$OHF>DwrsM3y3S+)4NlxXbeWvXB{TV~F8AaM z>Yg|Z($l~bL{5`?d8NPx%+bJQz%PxX(m2D|(Q)zy0a-kOETuIchr`Zk&BYESZXK>s)nJv;`6IAnqhaHYj<2Dp5hW!N&g#L!KSoq<7y6=&X@ Q%wseeyQQ1A7+Ej^041TBdjJ3c From b95634edf3bdf551c4bff5667fec8996fbdcdb3a Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Thu, 30 May 2024 13:58:13 +0200 Subject: [PATCH 10/13] Fixing some flaws inside the macro IDE * When deleting a macro, the tab was not closed but became "zombie" * After creating a new folder, macros made there behaved "zombie" too --- src/lay/lay/layMacroEditorDialog.cc | 36 +++++++++++------- src/lay/lay/layMacroEditorDialog.h | 2 + src/lay/lay/layMacroEditorTree.cc | 20 ++++++++++ src/lay/lay/layMacroEditorTree.h | 2 + src/lym/lym/lymMacroCollection.cc | 58 ++++++++++++++++++++++++++++- src/lym/lym/lymMacroCollection.h | 25 +++++++++++++ 6 files changed, 127 insertions(+), 16 deletions(-) diff --git a/src/lay/lay/layMacroEditorDialog.cc b/src/lay/lay/layMacroEditorDialog.cc index bf7e5df7d..ed07e8e17 100644 --- a/src/lay/lay/layMacroEditorDialog.cc +++ b/src/lay/lay/layMacroEditorDialog.cc @@ -278,7 +278,8 @@ MacroEditorDialog::MacroEditorDialog (lay::Dispatcher *pr, lym::MacroCollection m_edit_trace_index (-1), m_add_edit_trace_enabled (true), dm_refresh_file_watcher (this, &MacroEditorDialog::do_refresh_file_watcher), - dm_update_ui_to_run_mode (this, &MacroEditorDialog::do_update_ui_to_run_mode) + dm_update_ui_to_run_mode (this, &MacroEditorDialog::do_update_ui_to_run_mode), + dm_current_tab_changed (this, &MacroEditorDialog::do_current_tab_changed) { // Makes this dialog receive events while progress bars are on - this way we can set breakpoints // during execution of a macro even if anything lengthy is running. @@ -1759,13 +1760,12 @@ MacroEditorDialog::macro_deleted (lym::Macro *macro) std::map ::iterator page = m_tab_widgets.find (macro); if (page != m_tab_widgets.end ()) { - // disable the macro on the page - we'll ask for updates when the file - // watcher becomes active. So long, the macro is "zombie". - page->second->connect_macro (0); - m_tab_widgets.erase (page); + int index = tabWidget->indexOf (page->second); + if (index >= 0) { + tab_close_requested (index); + } } - refresh_file_watcher (); update_ui_to_run_mode (); } @@ -1792,12 +1792,10 @@ MacroEditorDialog::macro_changed (lym::Macro *macro) } } -void -MacroEditorDialog::current_tab_changed (int index) +void +MacroEditorDialog::do_current_tab_changed () { - add_edit_trace (false); - - MacroEditorPage *page = dynamic_cast (tabWidget->widget (index)); + MacroEditorPage *page = dynamic_cast (tabWidget->currentWidget ()); if (page) { int tab_index = 0; for (std::vector::const_iterator mt = m_macro_trees.begin (); mt != m_macro_trees.end (); ++mt, ++tab_index) { @@ -1807,9 +1805,19 @@ MacroEditorDialog::current_tab_changed (int index) } } } +} - replaceFrame->setEnabled (page && page->macro () && !page->macro ()->is_readonly ()); +void +MacroEditorDialog::current_tab_changed (int index) +{ + // select the current macro - done in a delayed fashion so there is + // no interacting during erase of macros + dm_current_tab_changed (); + add_edit_trace (false); + + MacroEditorPage *page = dynamic_cast (tabWidget->widget (index)); + replaceFrame->setEnabled (page && page->macro () && !page->macro ()->is_readonly ()); apply_search (); do_update_ui_to_run_mode (); @@ -2500,6 +2508,7 @@ BEGIN_PROTECTED if (m->is_readonly ()) { throw tl::Exception ("Can't delete this macro - it is readonly"); } + if (collection) { if (QMessageBox::question (this, QObject::tr ("Delete Macro File"), @@ -2512,12 +2521,11 @@ BEGIN_PROTECTED throw tl::Exception ("Can't delete this macro"); } + ct->set_current (collection); collection->erase (m); } - ct->set_current (collection); - } refresh_file_watcher (); diff --git a/src/lay/lay/layMacroEditorDialog.h b/src/lay/lay/layMacroEditorDialog.h index 7a0e2e2c5..001829924 100644 --- a/src/lay/lay/layMacroEditorDialog.h +++ b/src/lay/lay/layMacroEditorDialog.h @@ -271,6 +271,7 @@ protected slots: void close_many (int which_relative_to_current); void ensure_writeable_collection_selected (); void update_console_text (); + void do_current_tab_changed (); void start_exec (gsi::Interpreter *interpreter); void end_exec (gsi::Interpreter *interpreter); size_t id_for_path (gsi::Interpreter *interpreter, const std::string &path); @@ -360,6 +361,7 @@ protected slots: std::vector m_changed_files, m_removed_files; tl::DeferredMethod dm_refresh_file_watcher; tl::DeferredMethod dm_update_ui_to_run_mode; + tl::DeferredMethod dm_current_tab_changed; QMenu *mp_tabs_menu; }; diff --git a/src/lay/lay/layMacroEditorTree.cc b/src/lay/lay/layMacroEditorTree.cc index 207d7bdce..cb6507be5 100644 --- a/src/lay/lay/layMacroEditorTree.cc +++ b/src/lay/lay/layMacroEditorTree.cc @@ -123,7 +123,9 @@ MacroTreeModel::MacroTreeModel (QObject *parent, lay::MacroEditorDialog *dialog, : QAbstractItemModel (parent), mp_dialog (dialog), mp_parent (dialog), mp_root (root), m_category (cat) { connect (root, SIGNAL (macro_changed (lym::Macro *)), this, SLOT (macro_changed ())); + connect (root, SIGNAL (macro_about_to_be_deleted (lym::Macro *)), this, SLOT (macro_about_to_be_deleted (lym::Macro *))); connect (root, SIGNAL (macro_deleted (lym::Macro *)), this, SLOT (macro_deleted (lym::Macro *))); + connect (root, SIGNAL (macro_collection_about_to_be_deleted (lym::MacroCollection *)), this, SLOT (macro_collection_about_to_be_deleted (lym::MacroCollection *))); connect (root, SIGNAL (macro_collection_deleted (lym::MacroCollection *)), this, SLOT (macro_collection_deleted (lym::MacroCollection *))); connect (root, SIGNAL (macro_collection_changed (lym::MacroCollection *)), this, SLOT (macro_collection_changed ())); connect (root, SIGNAL (about_to_change ()), this, SLOT (about_to_change ())); @@ -133,7 +135,9 @@ MacroTreeModel::MacroTreeModel (QWidget *parent, lym::MacroCollection *root, con : QAbstractItemModel (parent), mp_dialog (0), mp_parent (parent), mp_root (root), m_category (cat) { connect (root, SIGNAL (macro_changed (lym::Macro *)), this, SLOT (macro_changed ())); + connect (root, SIGNAL (macro_about_to_be_deleted (lym::Macro *)), this, SLOT (macro_about_to_be_deleted (lym::Macro *))); connect (root, SIGNAL (macro_deleted (lym::Macro *)), this, SLOT (macro_deleted (lym::Macro *))); + connect (root, SIGNAL (macro_collection_about_to_be_deleted (lym::MacroCollection *)), this, SLOT (macro_collection_about_to_be_deleted (lym::MacroCollection *))); connect (root, SIGNAL (macro_collection_deleted (lym::MacroCollection *)), this, SLOT (macro_collection_deleted (lym::MacroCollection *))); connect (root, SIGNAL (macro_collection_changed (lym::MacroCollection *)), this, SLOT (macro_collection_changed ())); connect (root, SIGNAL (about_to_change ()), this, SLOT (about_to_change ())); @@ -144,11 +148,27 @@ Qt::DropActions MacroTreeModel::supportedDropActions() const return Qt::MoveAction; } +void MacroTreeModel::macro_about_to_be_deleted (lym::Macro *macro) +{ + QModelIndex index = index_for (macro); + if (index.isValid ()) { + changePersistentIndex (index, QModelIndex ()); + } +} + void MacroTreeModel::macro_deleted (lym::Macro *) { // .. nothing yet .. } +void MacroTreeModel::macro_collection_about_to_be_deleted (lym::MacroCollection *mc) +{ + QModelIndex index = index_for (mc); + if (index.isValid ()) { + changePersistentIndex (index, QModelIndex ()); + } +} + void MacroTreeModel::macro_collection_deleted (lym::MacroCollection *) { // .. nothing yet .. diff --git a/src/lay/lay/layMacroEditorTree.h b/src/lay/lay/layMacroEditorTree.h index 6d7a91606..69d616df6 100644 --- a/src/lay/lay/layMacroEditorTree.h +++ b/src/lay/lay/layMacroEditorTree.h @@ -69,7 +69,9 @@ Q_OBJECT private slots: void macro_changed (); + void macro_about_to_be_deleted (lym::Macro *macro); void macro_deleted (lym::Macro *macro); + void macro_collection_about_to_be_deleted (lym::MacroCollection *mc); void macro_collection_deleted (lym::MacroCollection *mc); void macro_collection_changed (); void about_to_change (); diff --git a/src/lym/lym/lymMacroCollection.cc b/src/lym/lym/lymMacroCollection.cc index c40645dd8..d91cc481b 100644 --- a/src/lym/lym/lymMacroCollection.cc +++ b/src/lym/lym/lymMacroCollection.cc @@ -124,6 +124,14 @@ void MacroCollection::on_macro_collection_changed (MacroCollection *mc) } } +void MacroCollection::on_child_about_to_be_deleted (MacroCollection *mc) +{ +#if defined(HAVE_QT) + emit child_about_to_be_deleted (mc); +#endif + on_macro_collection_about_to_be_deleted (mc); +} + void MacroCollection::on_child_deleted (MacroCollection *mc) { #if defined(HAVE_QT) @@ -132,6 +140,17 @@ void MacroCollection::on_child_deleted (MacroCollection *mc) on_macro_collection_deleted (mc); } +void MacroCollection::on_macro_collection_about_to_be_deleted (MacroCollection *mc) +{ + if (mp_parent) { + mp_parent->on_macro_collection_about_to_be_deleted (mc); + } else { +#if defined(HAVE_QT) + emit macro_collection_about_to_be_deleted (mc); +#endif + } +} + void MacroCollection::on_macro_collection_deleted (MacroCollection *mc) { if (mp_parent) { @@ -143,6 +162,14 @@ void MacroCollection::on_macro_collection_deleted (MacroCollection *mc) } } +void MacroCollection::on_macro_about_to_be_deleted_here (Macro *macro) +{ +#if defined(HAVE_QT) + emit macro_about_to_be_deleted_here (macro); +#endif + on_macro_about_to_be_deleted (macro); +} + void MacroCollection::on_macro_deleted_here (Macro *macro) { #if defined(HAVE_QT) @@ -151,6 +178,17 @@ void MacroCollection::on_macro_deleted_here (Macro *macro) on_macro_deleted (macro); } +void MacroCollection::on_macro_about_to_be_deleted (Macro *macro) +{ + if (mp_parent) { + mp_parent->on_macro_about_to_be_deleted (macro); + } else { +#if defined(HAVE_QT) + emit macro_about_to_be_deleted (macro); +#endif + } +} + void MacroCollection::on_macro_deleted (Macro *macro) { if (mp_parent) { @@ -539,8 +577,9 @@ void MacroCollection::erase (lym::Macro *mp) for (iterator m = m_macros.begin (); m != m_macros.end (); ++m) { if (m->second == mp) { begin_changes (); - on_macro_deleted_here (mp); + on_macro_about_to_be_deleted_here (mp); m_macros.erase (m); + on_macro_deleted_here (mp); delete mp; on_changed (); return; @@ -553,8 +592,9 @@ void MacroCollection::erase (lym::MacroCollection *mp) for (child_iterator f = m_folders.begin (); f != m_folders.end (); ++f) { if (f->second == mp) { begin_changes (); - on_child_deleted (mp); + on_child_about_to_be_deleted (mp); m_folders.erase (f); + on_child_deleted (mp); delete mp; on_changed (); return; @@ -608,11 +648,25 @@ bool MacroCollection::rename (const std::string &n) return false; } else { m_path = n; + if (parent ()) { + parent ()->folder_renamed (this); + } on_changed (); return true; } } +void MacroCollection::folder_renamed (MacroCollection *mc) +{ + for (auto f = m_folders.begin (); f != m_folders.end (); ++f) { + if (f->second == mc) { + m_folders.erase (f); + m_folders.insert (std::make_pair (mc->name (), mc)); + return; + } + } +} + lym::MacroCollection *MacroCollection::create_folder (const char *prefix, bool mkdir) { std::string name; diff --git a/src/lym/lym/lymMacroCollection.h b/src/lym/lym/lymMacroCollection.h index 31fde7e53..28c851b08 100644 --- a/src/lym/lym/lymMacroCollection.h +++ b/src/lym/lym/lymMacroCollection.h @@ -463,21 +463,41 @@ Q_OBJECT */ void changed (); + /** + * @brief This signal is sent by collection when a child collection is about to be deleted in this collection + */ + void child_about_to_be_deleted (lym::MacroCollection *); + /** * @brief This signal is sent by collection when a child collection is deleted in this collection */ void child_deleted (lym::MacroCollection *); + /** + * @brief This signal is sent by the root object when a macro collection is about to be deleted + */ + void macro_collection_about_to_be_deleted (lym::MacroCollection *); + /** * @brief This signal is sent by the root object when a macro collection is deleted */ void macro_collection_deleted (lym::MacroCollection *); + /** + * @brief This signal is sent by collection when a macro is about to be deleted in this collection + */ + void macro_about_to_be_deleted_here (lym::Macro *); + /** * @brief This signal is sent by collection when a macro is deleted in this collection */ void macro_deleted_here (lym::Macro *); + /** + * @brief This signal is sent by the root object when a macro is about to be deleted + */ + void macro_about_to_be_deleted (lym::Macro *); + /** * @brief This signal is sent by the root object when a macro is deleted */ @@ -522,9 +542,13 @@ Q_OBJECT int m_virtual_mode; bool m_readonly; + void on_child_about_to_be_deleted (MacroCollection *mc); void on_child_deleted (MacroCollection *mc); + void on_macro_collection_about_to_be_deleted (MacroCollection *mc); void on_macro_collection_deleted (MacroCollection *mc); + void on_macro_about_to_be_deleted_here (Macro *macro); void on_macro_deleted_here (Macro *macro); + void on_macro_about_to_be_deleted (Macro *macro); void on_macro_deleted (Macro *macro); void on_macro_changed (Macro *macro); void on_macro_collection_changed (MacroCollection *mc); @@ -535,6 +559,7 @@ Q_OBJECT void create_entry (const std::string &path); void rename_macro (Macro *macro, const std::string &new_name); + void folder_renamed (MacroCollection *mc); void begin_changes (); From 11bddc2914f8bfca7913f2b2e1b3a4cdea22ce8f Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Fri, 31 May 2024 14:44:56 +0200 Subject: [PATCH 11/13] Refining solution for "blend-mode 0" enhancement --- src/db/db/dbCommonReader.cc | 48 ++++++++++++++++++++++---------- src/db/db/dbCommonReader.h | 2 +- src/db/db/dbInstances.cc | 5 ++++ testdata/gds/collect_add_au.gds | Bin 13292 -> 13266 bytes 4 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/db/db/dbCommonReader.cc b/src/db/db/dbCommonReader.cc index fb4f5dfe7..108a62851 100644 --- a/src/db/db/dbCommonReader.cc +++ b/src/db/db/dbCommonReader.cc @@ -164,7 +164,7 @@ CommonReaderBase::rename_cell (db::Layout &layout, size_t id, const std::string // Both cells already exist and are not identical: merge ID-declared cell into the name-declared one layout.force_update (); - merge_cell (layout, iname->second.second, iid->second.second, true); + merge_cell (layout, iname->second.second, iid->second.second, true, false); iid->second.second = iname->second.second; } @@ -239,24 +239,44 @@ CommonReaderBase::cell_for_instance (db::Layout &layout, const std::string &cn) } void -CommonReaderBase::merge_cell (db::Layout &layout, db::cell_index_type target_cell_index, db::cell_index_type src_cell_index, bool with_meta) const +CommonReaderBase::merge_cell (db::Layout &layout, db::cell_index_type target_cell_index, db::cell_index_type src_cell_index, bool with_meta, bool no_duplicate_instances) const { const db::Cell &src_cell = layout.cell (src_cell_index); db::Cell &target_cell = layout.cell (target_cell_index); target_cell.set_ghost_cell (src_cell.is_ghost_cell () && target_cell.is_ghost_cell ()); - // avoid generating duplicates - std::set current; - for (db::Cell::const_iterator i = target_cell.begin (); ! i.at_end (); ++i) { - current.insert (*i); - } + if (no_duplicate_instances) { + + // avoid generating duplicates + std::set current; + for (db::Cell::const_iterator i = target_cell.begin (); ! i.at_end (); ++i) { + current.insert (*i); + } - // copy over the instances - for (db::Cell::const_iterator i = src_cell.begin (); ! i.at_end (); ++i) { - // NOTE: cell indexed may be invalid because we delete subcells without update() - if (layout.is_valid_cell_index (i->cell_index ()) && current.find (*i) == current.end ()) { - target_cell.insert (*i); + // copy over the instances + // NOTE: need to do that in a two-step fashion as inserting instances may invalidate + // the existing ones. + std::vector selected; + for (db::Cell::const_iterator i = src_cell.begin (); ! i.at_end (); ++i) { + // NOTE: cell indexed may be invalid because we delete subcells without update() + selected.push_back (layout.is_valid_cell_index (i->cell_index ()) && current.find (*i) == current.end ()); } + auto s = selected.begin (); + for (db::Cell::const_iterator i = src_cell.begin (); ! i.at_end (); ++i, ++s) { + if (*s) { + target_cell.insert (*i); + } + } + + } else { + + for (db::Cell::const_iterator i = src_cell.begin (); ! i.at_end (); ++i) { + // NOTE: cell indexed may be invalid because we delete subcells without update() + if (layout.is_valid_cell_index (i->cell_index ())) { + target_cell.insert (*i); + } + } + } merge_cell_without_instances (layout, target_cell_index, src_cell_index, with_meta); @@ -391,7 +411,7 @@ CommonReaderBase::finish (db::Layout &layout) layout.cell (ci_org).clear_shapes (); - merge_cell (layout, ci_org, ci_new, true); + merge_cell (layout, ci_org, ci_new, true, false); } else if (m_cc_resolution == SkipNewCell && ! layout.cell (ci_org).is_ghost_cell ()) { @@ -403,7 +423,7 @@ CommonReaderBase::finish (db::Layout &layout) } else { - merge_cell (layout, ci_org, ci_new, m_cc_resolution != SkipNewCell); + merge_cell (layout, ci_org, ci_new, m_cc_resolution != SkipNewCell, m_cc_resolution == AddToCell); } diff --git a/src/db/db/dbCommonReader.h b/src/db/db/dbCommonReader.h index 90e98d339..d8749c368 100644 --- a/src/db/db/dbCommonReader.h +++ b/src/db/db/dbCommonReader.h @@ -242,7 +242,7 @@ class DB_PUBLIC CommonReaderBase /** * @brief Merge (and delete) the src_cell into target_cell */ - void merge_cell (db::Layout &layout, db::cell_index_type target_cell_index, db::cell_index_type src_cell_index, bool with_meta) const; + void merge_cell (db::Layout &layout, db::cell_index_type target_cell_index, db::cell_index_type src_cell_index, bool with_meta, bool no_duplicate_instances) const; /** * @brief Merge (and delete) the src_cell into target_cell without instances diff --git a/src/db/db/dbInstances.cc b/src/db/db/dbInstances.cc index 4f1a8ca54..3ad24f8c1 100644 --- a/src/db/db/dbInstances.cc +++ b/src/db/db/dbInstances.cc @@ -539,6 +539,11 @@ template class instance_iterator; // ------------------------------------------------------------------------------------- // NormalInstanceIteratorTraits implementation +// TOOD: this class could use standard iterators instead of flat +// box tree ones. This potentially saves a sorting step when +// no box trees are needed and order will remain more stable in +// that case. + NormalInstanceIteratorTraits::NormalInstanceIteratorTraits () : mp_insts (0) { } diff --git a/testdata/gds/collect_add_au.gds b/testdata/gds/collect_add_au.gds index b021521a81c58832235cef4463c2658404635fe8..c18d23dff49bf27781dfd8840a97f179c1821a70 100644 GIT binary patch delta 909 zcmaEpekomvfsKKQDS|*ZfQ4xFn8nbab4&K|mH~KuT#X#Oj=LTJy1rH)ub{Dqf|t z6RWtJ?p>_nW_sAoG0;DWRXvY^F;?*)1MDeln*nxzWf@{m(L6>|v6;Vli;)E*01}Ls A&j0`b delta 902 zcmcbV{w7_CfsKKQDS|uR?P`I zJF$xE=-$ODE~EDxtGJl{Nvz_Z^s)M7vV{TmBsIqXyC(w-u_xtEhEuWGyLpz81tS1W CD2=uN From 2e7e299af98e1fecfc11647d3b8645c8fa03f53a Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Fri, 31 May 2024 15:16:44 +0200 Subject: [PATCH 12/13] Comment typo fixed --- src/db/db/dbInstances.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/db/db/dbInstances.cc b/src/db/db/dbInstances.cc index 3ad24f8c1..4d4c3c76e 100644 --- a/src/db/db/dbInstances.cc +++ b/src/db/db/dbInstances.cc @@ -539,7 +539,7 @@ template class instance_iterator; // ------------------------------------------------------------------------------------- // NormalInstanceIteratorTraits implementation -// TOOD: this class could use standard iterators instead of flat +// TODO: this class could use standard iterators instead of flat // box tree ones. This potentially saves a sorting step when // no box trees are needed and order will remain more stable in // that case. From 3902ad65765f9fc9967b02f5af6b5f94dc732e28 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Fri, 31 May 2024 15:59:32 +0200 Subject: [PATCH 13/13] Removing a TODO - that suggestion isn't good as of now --- src/db/db/dbInstances.cc | 5 ----- src/db/unit_tests/dbCellTests.cc | 4 ++++ 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/db/db/dbInstances.cc b/src/db/db/dbInstances.cc index 4d4c3c76e..4f1a8ca54 100644 --- a/src/db/db/dbInstances.cc +++ b/src/db/db/dbInstances.cc @@ -539,11 +539,6 @@ template class instance_iterator; // ------------------------------------------------------------------------------------- // NormalInstanceIteratorTraits implementation -// TODO: this class could use standard iterators instead of flat -// box tree ones. This potentially saves a sorting step when -// no box trees are needed and order will remain more stable in -// that case. - NormalInstanceIteratorTraits::NormalInstanceIteratorTraits () : mp_insts (0) { } diff --git a/src/db/unit_tests/dbCellTests.cc b/src/db/unit_tests/dbCellTests.cc index 5715d1053..0cffd31e6 100644 --- a/src/db/unit_tests/dbCellTests.cc +++ b/src/db/unit_tests/dbCellTests.cc @@ -998,6 +998,10 @@ TEST(6) // Note: iterating and replace does not work in non-editable mode if (db::default_editable_mode ()) { + std::vector insts; + for (db::Cell::const_iterator i = cc.begin (); ! i.at_end (); ++i) { + insts.push_back (*i); + } for (db::Cell::const_iterator i = cc.begin (); ! i.at_end (); ++i) { cc.replace_prop_id (*i, i->prop_id () + 1); }